Moodle
  1. Moodle
  2. MDL-42040

Organize the registration of shutdown handlers

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.6
    • Fix Version/s: 2.6
    • Component/s: General, Libraries
    • Labels:
    • Testing Instructions:
      Hide

      Note: all problem info goes the the error log because the page footer is already sent to output

      1/ test database sessions work (turn editing mode in course on and off)
      2/ verify profiling works (see description or ask Eloy how to set it up)
      3/ general install/upgrade/unittests

      Show
      Note: all problem info goes the the error log because the page footer is already sent to output 1/ test database sessions work (turn editing mode in course on and off) 2/ verify profiling works (see description or ask Eloy how to set it up) 3/ general install/upgrade/unittests
    • Affected Branches:
      MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w41_MDL-42040_m26_shutdown
    • Story Points:
      4
    • Rank:
      53224
    • Sprint:
      BACKEND Sprint 5

      Description

      This issue arrived because, recently I've started to get some "$DB not available" messages when profiling pages, leading to the profiling runs not sent to DB. And I'm pretty sure that it has been working ok since 2.0, until we introduced the new sessions stuff that changed the internal order of those handlers.

      So please, avoid arguing things like "it should not be using $DB" and friends (I'm aware of them). It worked and now it does not. Let's try to improve the situation and the future.

      Right now the registration of shutdown handlers (register_shutdown_function) is completely chaotic and disorganized, and that can lead to more and more problems where some objects, not available anymore, or disposed by a previous handler are needed later in the chain of shutdowns.

      grep -lr register_shutdown_function *
      

      So we need some method to organize and force their registration in an ordered way, declaring their needs (DB, logging, whatever) and observing any dependencies between them and also, surely containing references to all the needed used to avoid its auto-disposal by php.

      Not sure how to encapsulate all that information (needs, dependencies, disposal) nor how to behave (multiple handlers vs a big one in charge of everything).

      But it should be computable, I hope. Ciao

        Activity

        Hide
        Eloy Lafuente (stronk7) added a comment -

        Perhaps some sort of shutdown_scheduler, allowing the registration of arbitrary functions with their needs and disposals may be ok. Something like (very basic approach, sure it can me moved to statics):

        $moodle_shutdown = new shutdown_scheduler();

        and, along code do calls like:

        $moodle_shutdown->register (callback, requires, disposes);

        And, at the end, the unique shutdown function (created in the scheduler constructor) will sort the callbacks based on requires and disposes.

        Just a quick idea, feel free. Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Perhaps some sort of shutdown_scheduler, allowing the registration of arbitrary functions with their needs and disposals may be ok. Something like (very basic approach, sure it can me moved to statics): $moodle_shutdown = new shutdown_scheduler(); and, along code do calls like: $moodle_shutdown->register (callback, requires, disposes); And, at the end, the unique shutdown function (created in the scheduler constructor) will sort the callbacks based on requires and disposes. Just a quick idea, feel free. Ciao
        Hide
        Petr Škoda added a comment -

        Thanks for the report, submitting for peer review.

        Show
        Petr Škoda added a comment - Thanks for the report, submitting for peer review.
        Hide
        Frédéric Massart added a comment -

        Hi Petr,

        I can't seem to find anything wrong in the patch, I just noticed 2 indentation issues in shutdown_manager:124, and cssparser:23. Also, I assome that it's not necessary to keep a reference to $DB or $CFG during core_shutdown_manager:: initialize(), could you comment on that?

        I just noticed, in moodle_database, we do not call dispose() any more, which means that the temptables and database_manager are not disposed either. Shouldn't that be done in request_shutdown()? I saw your comment about legacy shutdown handlers, but then how do we deal with that previous logic? Wasn't it impossible to register a shutdown handler BEFORE the DB one (except when NO_MOODLE_COOKIES)?

        Cheers,
        Fred

        Show
        Frédéric Massart added a comment - Hi Petr, I can't seem to find anything wrong in the patch, I just noticed 2 indentation issues in shutdown_manager:124, and cssparser:23. Also, I assome that it's not necessary to keep a reference to $DB or $CFG during core_shutdown_manager:: initialize(), could you comment on that? I just noticed, in moodle_database, we do not call dispose() any more, which means that the temptables and database_manager are not disposed either. Shouldn't that be done in request_shutdown()? I saw your comment about legacy shutdown handlers, but then how do we deal with that previous logic? Wasn't it impossible to register a shutdown handler BEFORE the DB one (except when NO_MOODLE_COOKIES)? Cheers, Fred
        Hide
        Petr Škoda added a comment -

        1/ whitespace fixed
        2/ the DB->dispose() is called from __destroy()

        submitting for integration, thanks

        Show
        Petr Škoda added a comment - 1/ whitespace fixed 2/ the DB->dispose() is called from __destroy() submitting for integration, thanks
        Hide
        Dan Poltawski added a comment -

        The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        TIA and ciao

        Show
        Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
        Hide
        Sam Hemelryk added a comment -

        Thanks Petr - this has been integrated now.

        Show
        Sam Hemelryk added a comment - Thanks Petr - this has been integrated now.
        Hide
        Adrian Greeve added a comment -

        Finally finished!

        • database sessions work
        • profiling still works
          • general install - no problem
          • upgrade - no problem
          • unit tests - no problem.

        Test passed.

        Show
        Adrian Greeve added a comment - Finally finished! database sessions work profiling still works general install - no problem upgrade - no problem unit tests - no problem. Test passed.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Yes, it's happening (somewhere in the French Polynesia, right now). And you did it, raising Moodle to new excellency levels.

        Or, if you prefer, yes, you fixed that boring issue.

        Thanks anyway! Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Yes, it's happening (somewhere in the French Polynesia, right now). And you did it, raising Moodle to new excellency levels. Or, if you prefer, yes, you fixed that boring issue. Thanks anyway! Ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Agile