Stop doing complicated things in a destructor. This is unsafe, because object properties can be destroyed before the object that holds them, so there are a lot of ways for this to randomly fail. Use a shutdown function instead. Register sessions in a container and have the container respond to shutdown.

Yes, please. Doing work in a destructor is worse than doing work in a constructor. Especially when it means writing to a persistance layer.

Stop doing complicated things in a destructor. This is unsafe, because object properties can be destroyed before the object that holds them, so there are a lot of ways for this to randomly fail. Use a shutdown function instead. Register sessions in a container and have the container respond to shutdown.

The object destruction order is only undefined during shutdown, and the destructor does not trigger any action during shutdown (see the shutdown property of SessionBackend), so I don't think anything unsafe is happening there.

Sessions are in a container (SessionManager::$allSessionBackends) but aren't preserved until the end of the request (and so the state to be saved isn't available during shutdown); it's basically a hand-crafted WeakMap. I think doing so would be a lot more problematic for PHPUnit (although probably a good thing otherwise, since it would reduce the number of session store writes per request).

Remove the username validity check, just store the name unconditionally. Validity can be the concern of some other layer.

Yeah not sure what was the idea with that. Validating on save is bad, whether or not it happens in the destructor. It makes the behavior inconsistent (getUser() will return a different user initially and after saving and restoring the session).

SessionBackend construction is centralized, it always happens in SessionManager::getSessionFromInfo(), so we should move the check there.

Fix the reference cycle in REST handlers so that they don't keep Session objects alive past the end of the relevant test.

I think you'll need to do this anyway because there can be various other kinds of trouble with keeping objects around across tests (e.g. if something gets mocked and the mock persists and gets called inside a different TestCase than the one it was created for, you get all kinds of weird errors).

In T400113#11022660, @daniel wrote:

Doing work in a destructor is worse than doing work in a constructor. Especially when it means writing to a persistance layer.

I don't see why, it's the standard way in PHP to implement a RAII pattern. We use it all around the place via scoped callbacks. It's relatively easy to ensure that some separate initialization method always gets called after object construction, it's really hard to ensure similar behavior for object destruction. You shouldn't rely on destructors during request shutdown, but that's a separate issue.

In T400113#11023851, @Tgr wrote:

In T400113#11022660, @daniel wrote:

Doing work in a destructor is worse than doing work in a constructor. Especially when it means writing to a persistance layer.

I don't see why, it's the standard way in PHP to implement a RAII pattern. We use it all around the place via scoped callbacks.

If this happens immediately on a local object when the current scope ends, fine - though I'd still prefer a scoped callback over a destructor. But for an object that is passsed around, stored in members, and bound to global scope, it seems problematic. Becuase the destructor may be called at a (seemingly) random time, in an unpredictable transactional context.

Also, I'd say freeing resources is fine in a constructor, but flushing/writing data isn't.

In any case, doing work in the destructor makes the control flow hard to reason about. This error is an example. It was reported as a circular dependency of service initialization, which it isn't - it's caused by a constructor getting called implicitoly, invisibly, at an apparently random time, without a hint to this in the code:

/workspace/src/includes/session/SessionBackend.php:232
/workspace/src/includes/session/Session.php:78
/workspace/src/includes/WikiMap/WikiMap.php:309
/workspace/src/includes/WikiMap/WikiMap.php:317

The step from WikiMap to Session is magic. WikiMap does not mention anything related to Sessions. Apparently GC just decided to kick in after returning from getCurrentWikiDbDomain(). That makes for really odd bugs...

It's relatively easy to ensure that some separate initialization method always gets called after object construction, it's really hard to ensure similar behavior for object destruction.

For persistenting the session, I think we wouldn't rely on that. We should do it explicitly in the shutdown code.

You shouldn't rely on destructors during request shutdown, but that's a separate issue.

Isn't that the critical point for session persistence?

In T400113#11023789, @Tgr wrote:

Sessions are in a container (SessionManager::$allSessionBackends) but aren't preserved until the end of the request (and so the state to be saved isn't available during shutdown); it's basically a hand-crafted WeakMap. I think doing so would be a lot more problematic for PHPUnit (although probably a good thing otherwise, since it would reduce the number of session store writes per request).

That should probably be in MediaWikiIntegrationtestBase.

Fix the reference cycle in REST handlers so that they don't keep Session objects alive past the end of the relevant test.

I think you'll need to do this anyway because there can be various other kinds of trouble with keeping objects around across tests (e.g. if something gets mocked and the mock persists and gets called inside a different TestCase than the one it was created for, you get all kinds of weird errors).

It should probably be done for all tests, not just REST.

In T400113#11023789, @Tgr wrote:

Remove the username validity check, just store the name unconditionally. Validity can be the concern of some other layer.

Yeah not sure what was the idea with that.

Maybe it's for anonymous users? Telling whether a User object is truly anonymous (vs. representing a named user with no local account) is frustratingly hard in MediaWiki.
We can replace it with a static IPUtils call, then.

In T400113#11023980, @daniel wrote:

It was reported as a circular dependency of service initialization, which it isn't - it's caused by a constructor getting called implicitoly, invisibly, at an apparently random time, without a hint to this in the code

Session.php:78 is Session::_destruct(), nothing invisible about it. The thing that actually makes that stack trace hard to understand is the lack of method names. That's a stupid thing PHPUnit does. We should probably figure out how to fix it.

The step from WikiMap to Session is magic.

Yeah, not sure what's up with that. For scoped callbacks, we rely on the fact that objects are destructed predictably when there cease to be references for them, and that works pretty reliably, so I doubt this is just the GC being invoked at some random point in time. I imagine it is somehow related to the DatabaseDomain object getting destructed, but no idea how.

For persistenting the session, I think we wouldn't rely on that. We should do it explicitly in the shutdown code.

De facto it happens there anyway, because the main SessionBackend will be anchored to the WebRequest in the RequestContext singleton, and will only be destructed when RequestContext is (and by that point the shutdown callbacks have already run and the sessions are already saved). There might be other session instances related to FauxRequest etc, but there aren't many of those in a normal request, and probably they aren't authenticated / persisted so saving them will be a noop.

We could make SessionManager hold onto all SessionBackend objects until the end (I think we'd just need to get rid of SessionBackend::deregisterSession()). The disadvantage of that is that requests that churn through a lot of different sessions will use an arbitrarily large amount of memory, and will be less reliable if something breaks and prevents all those accumulated writes.

Outside of tests and the job runner, I don't think there's anything that would legitimately churn through lots of sessions. The job runner will go through a bunch of sessions, due to RequestContext::importScopedSession(), but I don't think there's a reason to save session data in a job runner - it can happen accidentally e.g. because of expiry, but if it fails, no big deal.

That leaves the memory issue. On my local machine, a SessionBackend instance uses about 5K memory, so probably not a big deal?

Change #1171720 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/core@master] Do not use UserNameUtils in SessionBackend::save()

https://gerrit.wikimedia.org/r/1171720

Change #1171720 merged by jenkins-bot:

[mediawiki/core@master] Do not use UserNameUtils in SessionBackend::save()

https://gerrit.wikimedia.org/r/1171720

Looks fixed, https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1139065 passed tests now.

Do we want to do anything about the session destructor, or do we close the task?

In T400113#11024528, @Tgr wrote:

We could make SessionManager hold onto all SessionBackend objects until the end (I think we'd just need to get rid of SessionBackend::deregisterSession()). The disadvantage of that is that requests that churn through a lot of different sessions will use an arbitrarily large amount of memory, and will be less reliable if something breaks and prevents all those accumulated writes.

We could also schedule a deferred update that will save the session. That will be called automatically at the end of the request.

Outside of tests and the job runner, I don't think there's anything that would legitimately churn through lots of sessions. The job runner will go through a bunch of sessions, due to RequestContext::importScopedSession(), but I don't think there's a reason to save session data in a job runner - it can happen accidentally e.g. because of expiry, but if it fails, no big deal.

For tests and jobs we have mechanisms to handle deferred updates.

That leaves the memory issue. On my local machine, a SessionBackend instance uses about 5K memory, so probably not a big deal?

Depends on how many of these we have - hundres would be fine, millions not so much :)