Avoid reading session from the database multiple times during a big pipe request

Two test failures, one is StandardPerformanceTest doing less database queries, the other is a json api test that needs investigation.

Tests should be green. Have to rebuild the container in the JSON API test, but the fact we only have to do that in a single test in core suggests this won't be disruptive in general.

Change of using a variable instead seems good to me.

Seems small enough that I can mark now vs waiting a week.

Rebased after 📌 Separate cache operations from database queries in OpenTelemetry and assertions Fixed

By adding a static cache are we increasing the changes of a race condition here, if a user has multiple page requests in flight and both of them want to write to the session?

I don't think it increases it or at least not compared to a non-bigpipe response.

The session is read right at the beginning of the request and that content is used until it's closed, so for a non-bigpipe response, it's only read once anyway, and anything that happens before the write would lead to a similar race condition. The static cache has no effect in this situation.

With big pipe, the issue is that we're reading the session, closing the session, then reading it again to close it again later, but the relative amount of the time between the session initially getting read and eventually getting written to isn't longer with bigpipe than it is with the 'normal' response. Another way to approach this would be to somehow make it so that the session doesn't get closed twice on bigpipe responses, that would mean no need for the static cache then, but I couldn't figure out any way to do that yet. Might look again though in case it's doable.

Another way to approach this would be to somehow make it so that the session doesn't get closed twice on bigpipe responses, that would mean no need for the static cache then, but I couldn't figure out any way to do that yet.

Thought of a way that seems OK, and completely gets rid of the need for the static cache.

Originally I was looking at decorating Drupal\Core\StackMiddleWare\Session but that would have led to a lot of duplicated code.

Instead, I added a new ResponseKeepSessionOpenInterface, have BigPipeResponse implement that, then have the session middleware check for it - when it's there, we skip closing the session after sending the response, which allows big pipe to take care of closing the session itself later, but without having to first reload it. That way we don't have to special-case big_pipe anywhere

catch → closed merge request !6142

Change makes sense and thanks to your performanceTest can see that 1 less call is made, so appears to be working.

Symfony's SessionInterface::save() says

     * This method is generally not required for real sessions as
     * the session will be automatically saved at the end of
     * code execution.

so do we have to explicitly save the session ourselves at all?

Pushed a commit to try that, but it doesn't look very encouraging - broke logins: https://git.drupalcode.org/project/drupal/-/pipelines/91673

Maybe worth exploring more in a follow-up - maybe we would just need to explicitly save the session when it's new or something like that but could be tricky.

Opened the follow-up 📌 Check if we can rely on Symfony to save the session for us Active .

Should we tag the new interface as @internal for now to try and discourage use, and then if we land 📌 Check if we can rely on Symfony to save the session for us Active then we can remove it again more easily?

That makes sense. Added @internal. It only exists to avoid special-casing BigPipeResponse in /core.

Committed and pushed 213906e286 to 11.x and 522404706e to 10.3.x. Thanks!

Another DB query bites the dust — nice one!

(CR seems to not be valuable here, because nothing else should need this, right?)

I'm curious to see if this will break https://www.drupal.org/project/big_pipe_sessionless → 😇 Happy to update that. Will know after tomorrow's scheduled CI pipeline 👍

I did originally think about a CR, but now we've made the interface internal I think we should explicitly not have one, and try to remove it again if we can rely on session auto saving.

Automatically closed - issue fixed for 2 weeks with no activity.

Tagging for release highlights, not enough by itself but combined with 📌 Optimize user logins by avoiding duplicate entity queries Needs work I think we can add something.