Problem/Motivation

Persistent app servers (such as FrankenPHP 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.

Proposed resolution

Symfony already has support for worker mode. Symfony keeps every service instance between requests by default, but has an opt-in reset list that resets services at the start of each main request after the first.

Drupal should basically do the same thing in a new major version (Drupal 13, hopefully?):

  • Require all services to be stateless or opt-in to resetting by tagging themselves with a "reset" tag.
  • This reset will happen in place: the same instance will be kept and its reset method will be called. This way, services that depend on it keep a valid reference.
  • Only reset services that were instantiated.

Remaining tasks

  • Work out the details of the approach.
    • How do we find services that have state that does not have a reset method: request data in properties, drupal_static(), or static caches?
    • How should we order resets?
    • Should the tag name reuse Symfony's kernel.reset with its method and on_invalid attributes?
  • Reach agreement on the tag name.
  • Implement the code and tests.
  • Tag the relevant core services.

API changes

  • New service tag.
  • The kernel calls reset methods on tagged, instantiated services before each main request after the first.

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: Reconnect and retry if the database connection goes away

#3494917: Reconnect and retry if the database connection goes away 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.

ptmkenny’s picture

ptmkenny commented

@catch and @ghost of drupal past:

Ok, you've convinced me :) I initially suggested the allow list because it was very easy to prototype, but when I actually tried to allow more services, I ran into a lot of trouble.

Symfony's EventDispatcher in particular looked to be very difficult to solve with an allow list, as Symfony seems to be designed to deny and reset everything in place.

I am now trying to follow what Symfony does, since they already have worker mode support, and seeing how we can adapt that to Drupal. I'll update the IS later after I understand it better.

ptmkenny’s picture

ptmkenny commented
Title: Allow services to declare whether they are safe to reuse across requests for persistent app servers » Create a reset list for services to enable use with persistent app servers
Issue summary: View changes

Old Problem/Motivation (reference only)

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.

ptmkenny’s picture

ptmkenny commented

Ok, I rewrote the IS to follow Symfony's approach. Also, I think this approach is workable because I have a prototype already sort-of working: https://github.com/ptmkenny/drupal_frankenphp_worker_mode/blob/main/patc...

What are your thoughts on the new IS?

ptmkenny’s picture

ptmkenny commented
Title: Create a reset list for services to enable use with persistent app servers » Let services declare a reset method for persistent app servers