Problem/Motivation

Persistent app servers (such as FrankenPHP in worker mode) handle tons of requests with one kernel. They do this by keeping services in memory, avoiding the need to bootstrap repeatedly. However, most of Drupal assumes that one bootstrap = one request. Between requests, core currently only offers two choices for handling instantiated services:

  • Keep all instantiated services. In this case, with a persistent app server, any service that stores request, user, or session data leaks it into the next request. Example: #3050383: PageCache cannot handle multiple main requests
  • Drop all instantiated services, with Container::reset() or DrupalKernel::resetContainer(), or by rebooting the kernel. This is safe, but then every request rebuilds the module handler, entity type manager, plugin managers, event dispatcher, and Twig environment, which removes most of the benefit of a persistent worker.

So, we need a way to identify which services are safe to keep, and which need to be reset for each request. There are a few ways we could do this. In #3324241: Provide DIC-friendly, concurrency-safe alternative(s) to `drupal_static()`, geek-merlin suggested an allow list:

Then add a blessed stateless_service tag, and only swap out the remaining services for a new request.

Something like $kernel->rebuildContainerForNewRequest(), as minimal API for now. And maybe in the end we can live with throwing away a handful of services.

Alternately, kim.pepper suggested collecting services that implement ResetInterface and resetting them (deny list approach), which is the approach that Symfony uses with the kernel.reset tag.

I think both approaches could work, but I favor an allow list because there are so many services in Drupal and if we allow by default, every service that may be stateful in contrib would leak data into subsequent requests, which seems like a security nightmare.

Proposed resolution

Add a way to declare which services are reused between requests and which are reset, using either an allow list or a deny list. I propose making "rebuilt every request" the default service lifetime, and letting services opt in to being reused.

Remaining tasks

  • Decide whether to do an allow list or deny list.
  • Work out the details of the approach. For example, a reused service can only depend on other reused services, so if using an allow list, we would need to enforce this somehow (compiler pass?).
  • Reach agreement on the tag name.
  • Implement the code and tests.
  • Tag the relevant core services.

API changes

  • New service tag.

Release notes snippet

TBD

Comments

ptmkenny created an issue. See original summary.

ptmkenny’s picture

ptmkenny commented
Title: Allow services to declare whether they safe to reuse across requests for persistent app servers » Allow services to declare whether they are safe to reuse across requests for persistent app servers
mfb’s picture

mfb
they or he
commented

Would there be a way to signal services to start a new runtime context? This is what I would need e.g. for the Sentry integration - it doesn't need to be reset in the sense of rebuilt, just provided a new runtime context or signaled to start a new runtime context.

ghost of drupal past’s picture

ghost of drupal past
he/him
commented

Yes, this would be great for temporal too.

Currently, the biggest issue temporal struggles with is the database connection going away between requests. The database API needs a driver dependent method which checks whether the exception thrown is such a one and if yes then the query needs to be automatically retried.

Until then, no service which holds the database connection can be marked safe which would render the issue moot :(

catch’s picture

catch
he/him
Primary language English
commented
Related issues: +#3494917: Database ping, close, and reconnect capabilities

#3494917: Database ping, close, and reconnect capabilities is open for that.

ghost of drupal past’s picture

ghost of drupal past
he/him
commented

Recommendation:

  1. Make this issue postponed on the connection one.
  2. Change this issue to blacklist. Almost all services are safe thanks to AccountProxy and RequestStack instead of actual user and request objects. I believe only if a service derives something else from request and holds that as a property you would have trouble which should be very rare.
ptmkenny’s picture

ptmkenny commented

@mfb Hmm, with an allow list, services would be rebuilt for the next request unless opted in (designated safe to keep), so there wouldn't be a signal. For a tagged service that implements ResetInterface, the kernel would call reset() either at the end of the request (terminate()) or at the start of the next main request, which would be an opportunity to do something. Is that enough, or do you need something else?

@ghost of drupal past: For point 1, I'm not sure that this issue should be blocked on the connection one; we need both, but they are separate scopes so hopefully they can be worked on in parallel.

Regarding your point 2, although "should be very rare" is what I was hoping for as well, testing FrankenPHP in worker mode over the past few days revealed a lot of services holding onto stuff that they should not be. I used Opus to quickly make a list of issues that I had to patch locally to get my behat and playwright tests to pass (initially, I didn't do any allow list or deny list; I just tried to fix everything):

  • PageCache middleware (#3050383: PageCache cannot handle multiple main requests): computed the cache ID from the first request and stored it in a property, so later requests used the wrong cache ID.
  • PathMatcher: $isCurrentFrontPage was computed from the first request's route match and never recomputed.
  • AliasPathMatcher: this decorator has its own $isCurrentFrontPage property with the same bug.
  • ThemeManager: the active theme negotiated from one request's route and user stayed active for later requests.
  • Block access control handler: access results are cached per account ID, but block visibility depends on the route and path, so later requests by the same account (e.g. anonymous) got the first request's results.
  • AccountProxy: AuthenticationSubscriber only calls setAccount() when an authentication provider applies, so an anonymous request after an authenticated one ran as the previous user.
  • Router request context (#3626006: DefaultExceptionHtmlSubscriber leaves the router with a detached GET request context after error-page subrequests): after an error-page subrequest, DefaultExceptionHtmlSubscriber left the router with a cloned request context whose method is forced to GET, instead of the shared request context.
  • SessionManager: after a session was destroyed, the storage state stayed out of sync with PHP, so the next request didn't start a new session.
  • Renderer: its render contexts kept references to earlier requests. These are stored in a static property, so rebuilding the renderer service does not clear them.

I am going to keep experimenting; now that I have tests passing, I've been fixing memory leaks and will next try to actually get a big increase in performance.

catch’s picture

catch
he/him
Primary language English
commented

Considering the number of services in core, the list in #7 looks relatively short to me. I would also expect that in at least some cases, we'd want to fix those rather than refreshing the services, making the deny list eventually even shorter.

ghost of drupal past’s picture

ghost of drupal past
he/him
commented

Most of this has a shape of "on kernel terminate needs to clean X out so the next request will start clean". This is better than recreating the service because service unclean propagates to the dependents and if AccountProxy and Renderer are unclean then the party is over before it started.

Changing Block access control handler caching logic is a separate issue, SessionManager is likely to be immense fun (if someone wants in, this SessionInitalizer class gives you a very concise overview of how session init happens with Symfony, the initialize methods of various bags create a reference into $_SESSION),
the rest is just subscribe to KernelEvents::TERMINATE and do some internal cleanup as far as I can see.