ReplicaKillSwitch unnecessarily starts a session on each request

The patch makes perfect sense to me :)

It should be possible to add a test for this?

Edit: encountered a different bug, please ignore.

One question though, does the bug described in the IS still exist for sites with a replica configured?

Let's add test coverage.

@Sam152 we have to start a session when there actually is a replica database, so it should be 'by design' in that case - the session value is the only thing that tells us to ignore the replica.

Is a session necessary though? Would the duration of the current request be sufficient?

I believe this issue is causing PHPUnit tests to fail on a client site I'm working on right now. The tests all pass in our local Docker env where there's only a single DB configured. On test instances on the client's hardware, there's a replica DB configured, and (some of) the tests fail with this error. If I remove the DB replica config from settings.local.php, the tests all pass. However, adding this patch doesn't help with the replica configured, since then the connection info count is > 1.

Therefore, I'm leaning towards the @kim.pepper question/solution: do we have to use a session for this at all, even in the replica case? Can't this trigger last the duration of the request, instead of trying to be time-based via sessions?

See also some additional related background / digging at #2941687: Subrequests in an KernelEvents::TERMINATE event subscriber (f.e. cron tasks doing subrequests) try to restart a session, resulting in runtime exceptions

@mglaman re: #10: Yes. See #2941687-17: Subrequests in an KernelEvents::TERMINATE event subscriber (f.e. cron tasks doing subrequests) try to restart a session, resulting in runtime exceptions where @EclipseGc writes:

Since this is using the session manager instead of consulting the request's cookies ParameterBag, it runs through the normal "does a session exist? no? start one" and then explodes. If this were to consult the request object embedded within the event, this would not happen. I'm unsure of the reasoning for using the session manager instead of consulting the request directly, however within \Drupal\Core\StackMiddleware\Session::handle() we explicitly ignore whether or not session data has been passed along in non-master request types.

Plus @kim.pepper's comments here and elsewhere.

So yes, flagging the replica ignore in the request object instead of using the session at all...

The session-based killswitch was originally added in Drupal 7 as db_ignore_slave(). I found the original issue in git history and there is some interesting reading here before we consider removing the time-based component of this and reducing it to the current request only:

#314358: Add option to ignore slave server for next requests
#302300: Support adding and ignoring targets

The ultimate source of this feature appears to be the following two blog posts:

https://dri.es/database-replication-lag
https://dri.es/scaling-with-mysql-replication

@mglaman: I didn't mean to imply you were being dense. I was just giving credit to the folks who influenced my thinking on this topic, lest anyone think I was claiming that solution as my own idea. ;)

@longwave: Very helpful, thanks! Will definitely read those threads ASAP.

Getting this error when trying to login to a site I recently updated:

RuntimeException: Failed to start the session because headers have already been sent by "/app/vendor/symfony/http-foundation/Response.php" at line 368. in Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage->start() (line 152 of /app/vendor/symfony/http-foundation/Session/Storage/NativeSessionStorage.php)
#0 /app/web/core/lib/Drupal/Core/Session/SessionManager.php(162): Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage->start()
#1 /app/web/core/lib/Drupal/Core/Session/SessionManager.php(193): Drupal\Core\Session\SessionManager->startNow()
#2 /app/vendor/symfony/http-foundation/Session/Session.php(189): Drupal\Core\Session\SessionManager->save()
#3 /app/web/core/lib/Drupal/Core/StackMiddleware/Session.php(60): Symfony\Component\HttpFoundation\Session\Session->save()
#4 /app/web/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(47): Drupal\Core\StackMiddleware\Session->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#5 /app/web/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(47): Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#6 /app/web/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(52): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#7 /app/vendor/stack/builder/src/Stack/StackedHttpKernel.php(23): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#8 /app/web/core/lib/Drupal/Core/DrupalKernel.php(716): Stack\StackedHttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#9 /app/web/index.php(19): Drupal\Core\DrupalKernel->handle(Object(Symfony\Component\HttpFoundation\Request))
#10 {main}
.

@dan2k3k4 Do you use redirect_after_login or a similar module? Please see #3214949: Headers have already been sent after upgrade to Drupal 9.2 (can't login)

@longwave ahh yep, thanks! I have that module installed and temporarily disabling it, fixed the issue for me. I had been Googling a bit the stack trace and ended up in this issue, my bad.

Hi, i have the same issue as @dan2k3k4 and i fixed the issue by disabling the "redirect after login" module.
this is the issue: https://www.drupal.org/project/redirect_after_login/issues/3214949
C.

Here's an implementation of a request-based killswitch.

Curently testing patch from #20 in my project. First signs seem promising.

I had to make one small change - On latest core (9.3.3) after applying the patch Drupal is throwing up Service "database.replica_kill_switch.request" must implement interface "Symfony\Component\EventDispatcher\EventSubscriberInterface" . Switching use statement to Symfony\Component\EventDispatcher\EventSubscriberInterface fixed it.

Fixed CS error.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

D10 Fails on this patch with

TypeError: Drupal\Core\Database\ReplicaKillSwitchRequest::__construct(): Argument #1 ($requestStack) must be of type Drupal\Core\Http\RequestStack, Symfony\Component\HttpFoundation\RequestStack given, called in /var/www/html/public/core/lib/Drupal/Component/DependencyInjection/Container.php on line 259 in Drupal\Core\Database\ReplicaKillSwitchRequest->__construct() (line 26 of core/lib/Drupal/Core/Database/ReplicaKillSwitchRequest.php).

This patch should work for D11 as well.

Fixes the patch conflicts and adds a kernel test.

I think we need to work out how this would be used instead of the session version? Do we switch to it and deprecate? How do we support both?

Is there any BC concerns if we switch?

Moving to NW for issue summary update. And if there is a backwards compatibility concern to switch?

I have reviewed some of the patches, including MR!25 and I do not think any of them address the fundamental problem: if the session has been ended, the ReplicaKillSwitch service is still going to attempt to start a session.

In our (extreme edge) case, we are attempting to save some entities in a shutdown function. The session has been closed, our shutdown function then saves a pending entity change, SqlContentEntityStorage::save then calls \Drupal::service('database.replica_kill_switch')->trigger() which then ends up creating an Exception which then causes a transaction rollback (inside SqlContentEntityStorage::save) and our attempt to save changes to an entity (in the shutdown function) are lost.

Incidentally, we don't have a replica server, but we do have another database connection. So our Database::getConnectionInfo count is 2.

I think the right solution here might be to check if the session ::isStarted before attempting to get from it or set data to it?

The theory behind starting the session when this was originally added is that if someone has just triggered saving an entity, then while the replica is catching up to the primary database, you want that user to see fresh data while they're browsing around the site, and not potentially stale data from the replica.

We could change that logic, and that would mean that the user potentially sees stale data until things catch up again. However, this was all added before render caching, and if anyone browses a listing query that's from a replica database listing stale data, then it'll get cached for other people too, and then this logic does nothing useful anyway. So I'm not sure it makes any sense any longer. Sites using this feature just have to deal with the possibility that some state listing data might get cached sometimes.

I think it's completely reasonable to avoid setting anything to the session unless the session is open, an actual user triggering an entity save without a session open would be extremely rare. So that might also be fine.

The MR will need to be targeted against 11.x, and it looks like the branch needs a rebase.

I pushed up my changes on https://git.drupalcode.org/issue/drupal-3181946/-/tree/3181946--replicak...

I tried to create my new branch off of 11.x, but things seemed to go awry and that's probably not the correct way to get this updated for 11.x. When I created the MR, it showed thousands of changes even though the new MR was targeting 11.x.

Someone with better git.drupalcode.org skills than me is needed to clean things up.

When you branch in an issue fork, it uses 11.x from the issue fork, and that can be very old on an old issue - so you would need to rebase the branch with origin 11.x again.

Thinking about alternatives to session - one possible option that would also fix caching would be to write a timestamp to state instead of session, and then not allow anything to query the replica for x seconds after the timestamp. This would also prevent stale data from the replica going into the render cache too.

@catch I am wondering if my understanding of ReplicaKillSwitch is correct.

This is to make it so that MY queries, in this current request, go to the primary db server instead of any of the replicas. The idea being that my current request just made a change to the database. That change goes through the primary connection. If my current request, being processed in the same thread, has a select query, the select goes through the primary connection again so that it is assured to have the updated data?

Then, because the kill switch is in my session for a period of time, my next request to load and display the updated node (and update entity & render caches) is guaranteed to get fresh data from the primary because the kill switch is sending me there. I suppose there is a small risk that another user's request comes in at that moment and, because they don't have the kill switch, they get stale data from a replica and replace the invalidated cache items with stale data.

That's the right idea yes. Except loading a node never goes to the replica, in core I think the only places that support it are search and views, so the only place to get stale data would be listing queries.

However there could be dozens of pages on which any one node appears - taxonomy term listings etc. and it would be easy for another user to warm the cache of those pages.