Moodle
  1. Moodle
  2. MDL-27229

start_delegated_transaction break xmlrpc/soap because of print_debug() function

    Details

    • Rank:
      16904

      Description

      When a transaction is rollback, $DB->print_debug() function is called. This function is printing some "warning" text.

      Problem:
      In the case of the SOAP/XMLRPC Zend webservice, the servers eval the code returned by external functions. When these DB rolbback warning messages are passed to eval(), it breaks the web services.

      A solution could be to set the $DB in web service mode (new attribut to the DB class) and then the print_debug function would not return message if the attribut is set to true.

      Cheers.

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment -

          This is not a bug in DML layer, you need to explicitly rollback in the ws layer and the warning will go away.

          Show
          Petr Škoda added a comment - This is not a bug in DML layer, you need to explicitly rollback in the ws layer and the warning will go away.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Wow, Jerome and do we really need to add those 3 lines all the time, to all the possible exceptions?

          It looks like a good candidate to use some of theses (alternatives):

          1) Create one external_exception, child of moodle_exception, and make all the exceptions used in external stuff to be children of external_exception. Then make that exception able to look if is_transaction_started() and cause the rollback to happen. Hacky, not sure if exceptions should be able to abort transactions themselves. Although for an organizational POV, having all the external exceptions under a common umbrella sounds ok here.

          2) Enclose the whole execution of the external functions into one try-catch (higher level) and, if exception happens, look if there is any transaction started, rollback it and re-throw the exception (much what you're doing individually but only once).

          3) Use some custom error handler for the execution of external functions via web services, able to silently rollback anything in progress. Not sure if I like this idea. Of course this must keep all the functions to continue being executable from moodle core (without the webservices stuff, using the standard error handler then).

          4) Create some: external_util::throw_exception($exception) to be used everywhere and performing the rollback and re-throw. Note 'external_util' is an invented name, surely you already have some class providing utils/helpers to the external stuff.

          I feel myself inclined to use 2) in first term (if such unique point of execution exists in the ws layer) or 4) (although this is the external layer, worse than the ws one). And I don't know if 1) and 3) are crazy or maybe could be considered.

          Perhaps this needs a bit of sharing and discussion @ HQ or so, just in case someone else has a better idea.

          Hope it helps, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Wow, Jerome and do we really need to add those 3 lines all the time, to all the possible exceptions? It looks like a good candidate to use some of theses (alternatives): 1) Create one external_exception, child of moodle_exception, and make all the exceptions used in external stuff to be children of external_exception. Then make that exception able to look if is_transaction_started() and cause the rollback to happen. Hacky, not sure if exceptions should be able to abort transactions themselves. Although for an organizational POV, having all the external exceptions under a common umbrella sounds ok here. 2) Enclose the whole execution of the external functions into one try-catch (higher level) and, if exception happens, look if there is any transaction started, rollback it and re-throw the exception (much what you're doing individually but only once). 3) Use some custom error handler for the execution of external functions via web services, able to silently rollback anything in progress. Not sure if I like this idea. Of course this must keep all the functions to continue being executable from moodle core (without the webservices stuff, using the standard error handler then). 4) Create some: external_util::throw_exception($exception) to be used everywhere and performing the rollback and re-throw. Note 'external_util' is an invented name, surely you already have some class providing utils/helpers to the external stuff. I feel myself inclined to use 2) in first term (if such unique point of execution exists in the ws layer) or 4) (although this is the external layer, worse than the ws one). And I don't know if 1) and 3) are crazy or maybe could be considered. Perhaps this needs a bit of sharing and discussion @ HQ or so, just in case someone else has a better idea. Hope it helps, ciao
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Hi Eloy,
          1) I thought about that but it would change the exception name, and this solution would not BC as the <exception>xxx</exception> would change.
          2) we do not rollback when there is no transaction. So at the end the developer would need to write also many lines (try catch the entire transaction code then do the rollback). However I think this solution is shorter than my current implementation.
          3) funny enough all is related We als overrode the SOAP fault() because the existing exception handler functions (the servers already have them) didn't always catch the exception in SOAP because, if I remember well, SOAP intercepts them before you can.
          4) I thought about that too but it looks weird to me to replace a normal throw by a lib function call. Even though this solution seems the easier and the shortest to me. At the end as this solution require the developer to know about it, I thought the three lines were as good as one line. The developer would be throwing the exception as they expect to throw it. Plus it makes them understand when to do it (e.g. during transaction. It would not be as obvious when to throw or when to use the lib core function, even thought not very complicated.)

          I'll vote for 4). Something called: external_transaction_throw_exception()

          Show
          Jérôme Mouneyrac added a comment - - edited Hi Eloy, 1) I thought about that but it would change the exception name, and this solution would not BC as the <exception>xxx</exception> would change. 2) we do not rollback when there is no transaction. So at the end the developer would need to write also many lines (try catch the entire transaction code then do the rollback). However I think this solution is shorter than my current implementation. 3) funny enough all is related We als overrode the SOAP fault() because the existing exception handler functions (the servers already have them) didn't always catch the exception in SOAP because, if I remember well, SOAP intercepts them before you can. 4) I thought about that too but it looks weird to me to replace a normal throw by a lib function call. Even though this solution seems the easier and the shortest to me. At the end as this solution require the developer to know about it, I thought the three lines were as good as one line. The developer would be throwing the exception as they expect to throw it. Plus it makes them understand when to do it (e.g. during transaction. It would not be as obvious when to throw or when to use the lib core function, even thought not very complicated.) I'll vote for 4). Something called: external_transaction_throw_exception()
          Hide
          Jérôme Mouneyrac added a comment -

          Hum... before to keep going on this track I'd like to be more accurate with the problem. Without the fix, SOAP returns this error log:

          Potential coding error - active database transaction detected when disposing database:\n* line 118 of /user/externallib.php: call to moodle_database->start_delegated_transaction()\n* line 286 of /webservice/lib.php(1131) : eval()'d code: call to core_user_external::create_users()\n* line ? of unknownfile: call to webservices_virtual_class_000000->core_user_create_users()\n* line 129 of /webservice/soap/locallib.php: call to SoapServer->handle()\n* line 1035 of /webservice/lib.php: call to moodle_zend_soap_serv . er->handle()\n* line 46 of /webservice/soap/server.php: call to webservice_zend_server->run()
          

          However SOAP doesn't break as mentioned in the description, it still returns the right exception message.

          With the fix, no error log.

          Looking a bit more to it...

          Show
          Jérôme Mouneyrac added a comment - Hum... before to keep going on this track I'd like to be more accurate with the problem. Without the fix, SOAP returns this error log: Potential coding error - active database transaction detected when disposing database:\n* line 118 of /user/externallib.php: call to moodle_database->start_delegated_transaction()\n* line 286 of /webservice/lib.php(1131) : eval()'d code: call to core_user_external::create_users()\n* line ? of unknownfile: call to webservices_virtual_class_000000->core_user_create_users()\n* line 129 of /webservice/soap/locallib.php: call to SoapServer->handle()\n* line 1035 of /webservice/lib.php: call to moodle_zend_soap_serv . er->handle()\n* line 46 of /webservice/soap/server.php: call to webservice_zend_server->run() However SOAP doesn't break as mentioned in the description, it still returns the right exception message. With the fix, no error log. Looking a bit more to it...
          Hide
          Jérôme Mouneyrac added a comment -

          Decreasing the priority as I can't reproduce breaking SOAP call.

          Show
          Jérôme Mouneyrac added a comment - Decreasing the priority as I can't reproduce breaking SOAP call.
          Hide
          Jérôme Mouneyrac added a comment -

          I added a solution similar to PHPUNIT

          Show
          Jérôme Mouneyrac added a comment - I added a solution similar to PHPUNIT
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I'd keep this out for now and revisit it after release.

          I'm not convinced that hacking moodle_database is the way to stop that debugging to happen. IMO we should find some solution:

          1/ Doing its job at the WS layer (servers), not moodle_database nor externallib.
          2/ Automatically detecting ongoing transactions and rollbacking them. The most I think on it, the less I like the 4th solution above, because it's @ external layer, and I think it should be one level outer.

          Note I don't get your comment about solution 2) above "we do not rollback if there is no transaction". Of course we only rollback if there is one exception AND if there is one ongoing transaction.

          IMO 2) continues being the best alternative: It's in the WS layer and can be automated.

          But let's see after release...IMO, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I'd keep this out for now and revisit it after release. I'm not convinced that hacking moodle_database is the way to stop that debugging to happen. IMO we should find some solution: 1/ Doing its job at the WS layer (servers), not moodle_database nor externallib. 2/ Automatically detecting ongoing transactions and rollbacking them. The most I think on it, the less I like the 4th solution above, because it's @ external layer, and I think it should be one level outer. Note I don't get your comment about solution 2) above "we do not rollback if there is no transaction". Of course we only rollback if there is one exception AND if there is one ongoing transaction. IMO 2) continues being the best alternative: It's in the WS layer and can be automated. But let's see after release...IMO, ciao
          Hide
          Jérôme Mouneyrac added a comment - - edited

          I'll implement 2), no worries. In fact could we consider that all external functions are transactional, always rollback on exception and always commit at the end? There are some lost performances but it seems reasonable. The advantage is that the web service developer would not have to take care about transactions.

          Show
          Jérôme Mouneyrac added a comment - - edited I'll implement 2), no worries. In fact could we consider that all external functions are transactional, always rollback on exception and always commit at the end? There are some lost performances but it seems reasonable. The advantage is that the web service developer would not have to take care about transactions.
          Hide
          Jérôme Mouneyrac added a comment -

          I try again to make it breaks but transactions do not seem to print_debug() (i.e. echo) on rollback. I would consider closing this issue. Let me know if you think we should really encapsulate all ws exception into a main exception (the main exception throwing the same one and can do more stuff). I think it's not required.

          Show
          Jérôme Mouneyrac added a comment - I try again to make it breaks but transactions do not seem to print_debug() (i.e. echo) on rollback. I would consider closing this issue. Let me know if you think we should really encapsulate all ws exception into a main exception (the main exception throwing the same one and can do more stuff). I think it's not required.
          Hide
          Jérôme Mouneyrac added a comment -

          I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue.
          For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment

          Show
          Jérôme Mouneyrac added a comment - I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue. For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated: