Memory leak in session_set_user (forum digest hits memory limit)

https://github.com/tlevi/moodle/compare/mdl32379_23

updating and raising to critical; breaks forum digests on any large site.

Thanks for discovering that and providing a fix.

Adding this to next sprint

Thanks for spot-on patch, pushing for peer-review

Hi,
Patch looks good.
I wrote a simple script as proof of concept for this issue.
You can run the script attached with both with and without the patch. Without patch you can actually see the memory limit growing with each call to the function, where it is constant with the patch applied.

The reason for the increase is mainly because $_SESSION['user'] is a reference to original $user so all data that is added to $_SESSION['user'] is done to the original array. which inflates the original array.

+1 for integration.
Thanks

attaching the test script.
Edit the path and run from cli
Thanks

Thanks for the Review and Script, Ankit

Hi, this behaviour was intentional and the change will probably degrade performance on sites where the data fits into memory. The stuff is $USER is cached data, not bloat.

It isn't useful at all where the user is touched once and it is hardly expected or predictable behavior that an object would be jammed with magic caches by the callee.

Is there a real case where performance is degraded?

I can not talk about 2.x cron because I worked on older versions only, previously it was helping because it was not very efficient, maybe it was improved to not loop the users array multiple times in all modules.

In any case please do not touch session_set_user() and instead tweak cron_setup_user() only if necessary, the reason is there is no way to verify how it is used in existing code including contrib and this change might theoretically cause some very nasty regressions in session related code.

Hmm, I am still not persuaded we should blame session_set_user() or cron_setup_user() because they work consistently with the rest of session and accesslib API - we always add caches to $user record when submitted as function parameter. The forum cron should be imo blamed because it should not try to create the huge array of user records in the first place. This "over-optimisation" is most probably my fault, at that time it helped moodle.org, but I agree that is a sloppy coding style. Ideally we should store only user->id in array and fetch the user records from some fast cache, PHP with its crazy array implementation does not help much here. Possible workaround might be to just clone the user record right before calling cron_setup_user(), it the long term we should imho eliminate the huge user array completely.

In any case thanks a lot for the report, patch and especially your cooperation! I agree this needs to be somehow fixed in all supported stable branches asap.

Hmmm yes but forum cron may not be the only one afflicted with this problem; it would be nice to solve all in one place. If session_set_user or cron setup user is not changed, there should at least be some clear documentation that the object will be modified with heaps of extra data.

This is living in our stables for a while now so if there is any extra slowness or regression somewhere we'll know; so far nothing.

Aside - forum cron itself needs a revisit eventually, it is causing some pain too.

Hi everyone,

Thanks for the work - this has been integrated now.

Perhaps we should create a new issue to investigate the root cause.

Dan: did you read all my comments? Please revert it or at least move the cloning to cron_setup_user().

Grr, my apologies.

I did read your comments and I thought that the change had been made. But clearly didn't read the diff properly.

Sorry everyone, reverting.

Sorry for the noise. I've reverted this change - I shouldn't have integrated this.

Reopening.

No worries dan, at least we have one more person deeply interested in this issue. Which of the options do you like most:
1/ clone in session_set_user() - I am personally worried about this one
2/ clone in cron_setup_user() - it should resolve this forum problem, some optimisations where $user is used repeatedly will stop work (that is both good and bad depending on the optimisations)
3/ clone $user before cron_setup_user() in forum cron - this fixes only forum code
4/ we could add new parameter to cron_setup_user() for $cloneuser=true - forum would be resolved for now, other places that really want to use the caching could enable it

+1 for point 4 (Option to clone user and set it to true for forum (for now)).

I prefer 2) but I don't know example of where we are optimising to take advantage of this.

Where do we depend on this? see forum cron:

foreach ($users as $userto) {
    cron_setup_user($userto);
    ...
    foreach ($posts as $pid => $post) {
        cron_setup_user($userto, $course);
        ....
    }
}

the inner loop benefits from the caching because it does not have do the very expensive USER->access caching repeatedly for the same user.

Hmm, we might be actually caching the access data in another cache, but still user preferences necessary for messaging would be refetched. I am wondering if the clone is actually going to solve much because the accesslib caching would not be prevented by that much (see $ACCESSLIB_PRIVATE->accessdatabyuser[$userid]).

Rehmmm, maybe we could dedupe the role definitions in CLI scripts to reduce memory usage, it might save some memory, but it would consume CPU cycles.

I wish I had some real data for testing here...

Looking at this again, can we optimise this a bit more by removing:

1. Assignments and using actual variables.
   $userto = $users[$userid];
2. After setting the session variable, unset $userto and use $_SESSION['USER']

This will avoid any extra copies and will optimise memory.

What do you think Petr?

Rajesh: I do not understand what you are proposing.

Sorry Petr,
My Bad... was thinking in wrong direction.

I will go with 3) for now, and backport this. Hope that will solve this issue and acceptable by everyone.

Hi Rajesh,
->The patch looks good, but as Petr pointed out this will degrade performance in sites where all data fits into memory. But I don't think we have any alternative.
->User data is still cached in $ACCESSLIB_PRIVATE when the cap checks are made. Not sure if that is going to create problems or not, but for sure this needs to be tested with some large data before we can assume $ACCESSLIB_PRIVATE is not going to create any problem.
Thanks

Petr: what would 'real data' be? Need a DB from a large site with these problems?

Thanks Ankit,

Petr also mentioned about this, but I am not sure how to get around this. I think this can be a bottleneck, but not sure as I can't test this.

@Tony:
Can you please test the patch and see if this helps.

Sorry, don't have the test data and setup that I needed to track this anymore, and it is resolved very well for us so I have to move on.

though, FWIW, I think Petr's concerns are theoretical at best.

The patch is not ok, my point was to NOT clone in that inner loops.

Thanks Petr,

So it should be done like this?

foreach ($users as $userto) {
    cron_setup_user(clone $userto);
    ...
    foreach ($posts as $pid => $post) {
        cron_setup_user($userto, $course);
        ....
    }
}

and

foreach ($users as $userto) {
    cron_setup_user(clone $userto);
    ...
    foreach ($thesediscussions as $discussionid) {
        cron_setup_user($userto, $course);
        ....
    }
}

I am not sure, if cloning $userto for outer loop will help.
At this point, I can't find any duplicate as well for this issue. So rather then using half-way solution (which I am not sure at this point), I would propose to look at this later when MUC is done.

In the mean while, I will close this as "Will not fix", and open a sub-task for MDL-23754.

Hello Tony and Petr,

Is it fine with you, if I close this issue and open sub-task for MDL-23754. I am not sure about this half-way solution.

Rajesh,

Appreciate Petr's passion to find the most elegant solution possible, but this is a significant issue for many of our large clients right now, and the fix Tony has provided fixes that issue right now on all our large sites, with no identifiable side effects that we have seen after running this in real, live, large sites.

We'll continue to run this in our managed environments, but to me this would seem to be a good interim fix for a proven performance issue in core while a more 'perfect' solution is debated.

Thoughts?

Mark.

Thanks for the quick reply Mark,

Unfortunately, we don't have a large site, on which we can test this solution.
Is it possible for you to test attached patch and let us know if it helps.

>In the mean while, I will close this as "Will not fix"

"Will not fix" a bug that consistently causes cron to throw OOM errors and sends out duplicate mail on every single cron run until the error is caught? I get that MUC might be able to handle it, but MUC is still a far ways off and a possible solution in the future is no reason to ignore critical bugs now (and really, given that with enough users to send mail to, it will fatal error, every time, this should probably be a blocker).

Hi Netspot guys - I agree we shouldn't close this - lets try and work together for a solution

Let me sum it up: the original proposed "fix" would create performance regression in the inner loop in forum cron and it would not resolve the caching problems in accesslib and it would not completely prevent memory exhaustion in forum cron caused by the user array iteration and it might create regressions elsewhere. The added "clone" in outer loop should conserve the same amount of memory in forum cron as the original proposed patch.

My +10 to improve cron performance and lower memory consumption, I personally do not have access to any real large server or backup data from it. I wrote the original code and I agree it is far from optimal, workaround is to allow more memory in cron, that was one of the reasons why we introduced $CFG->extramemorylimit - large installs can not run with default settings, sorry.

There is no noticeable speed difference in our production environments.

What you are talking about is a theoretical micro-optimisation and has no application.

Unbounded memory use for no real reason is a bug, regardless of magical configuration options.

Just a thought around this that perhaps the forum loop would benefit from a gc_collect_cycles() to help clear up the memory.

The OU guys found this was necessary in MDL-32214

No, the memory is attached to real live objects; this won't help.

Tony: I did not say it is "not a bug", I was only explaining why the first proposed patch was problematic. It would be really great if somebody improved the forum cron or even completely redesigned the cron execution. Please note this is GPL code where you have no guarantees, if there are bugs you can not demand them to be fixed, but you can do whatever you want with your copy of the code (well, things that GNU GPL 3 allows you). If somebody comes with better fix I guess there are not going to be any problems integrating it into moodle distribution that is available from moodle.org and if the risks are small it may be applied to all stable branches.

Oh we have forked already, I just figured upstream wanted some contributions.

I see that throughly isn't the case, based purely on theoretical proven false objections yet again, so I'll leave you to it.

P.S. This here is how all good software projects die.

oh, and "What you are talking about is a theoretical micro-optimisation and has no application." was not true in case of moodle.org when the code was introduced (around 1.8), it may not be the case now, but to me it looks like the current USER->access init is more expensive than before, but again I have no idea how much the messaging costs now, how does the rendering work, etc. In any case integrators and testers need to have a way to prove the optimisation works, the attached sc.php file is imo not very relevant to cron because it does not use capabilities and messaging, it just illustrates PHP object reference behaviour. We should collect some hard data with memory use, number of db queries+their type, execution times on different site types...

Tony: I would like to thank you for all your contributions, it is really valuable and being appreciated. I am going to look at your other unresolved issues and try to help to get them through integration if possible.

I have reviewed the patch by Rajesh now and I agree it is not correct solution either, I am going to work on my own patch now, I would really appreciate if you helped me with testing it later.

here is the patch (untested), but it should show my direction: https://github.com/skodak/moodle/compare/w20_MDL-32379_m23_forumcron

new features:

• user cache size in cron is controlled by new constant that can be overridden from config.php
• the user records are cloned before setting up session user in the outer foreach loop
• smaller user record size by unsetting more properties that are not necessary

Submitted for integration, please test. I did not feel confident enough to request stable branches, but the code should cherry pick without problems if necessary. I hope this could solve it for now...

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

Wow, this had somehow escaped my integration radar. Sorry.

Integrated, thanks

5 users were sent post 19

Looks good in my testing, but indeed this sort of thing is quite difficult to test, hence master only.

Thanks everyone!

This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads).

Thanks!

There is a problem with moodle.org digests (MDLSITE-1834), still trying to diagnose this, but wonder if it could be caused by this issue.

Hi, just to note that i've setup a test RIG with 600,000 users and memory still doesn't seem to be scaling properly.

I am investigating in MDL-23687