Regression: All stack middlewares are constructed at the same time even for cached pages

Created on 28 July 2025, 6 days ago

Problem/Motivation

#2473113: All stack middlewares are constructed at the same time even for cached pages β†’ is back.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

πŸ› Bug report
Status

Active

Version

11.0 πŸ”₯

Component

base system

Created by

πŸ‡¨πŸ‡­Switzerland znerol

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @znerol
  • πŸ‡¨πŸ‡­Switzerland znerol

    I guess this could be solved with a service closure for the $http_kernel argument in PageCache middleware.

  • πŸ‡¨πŸ‡¦Canada Charlie ChX Negyesi 🍁Canada

    I reviewed the parent issue and I am not sure what is going on, I am unfamiliar with the lazy notation, the dumper sets $service['lazy'] = TRUE; but the word lazy does not appear in Drupal\Component\DependencyInjection\Container. Is there a problem with that?

  • πŸ‡¬πŸ‡§United Kingdom catch

    #2 looks like it should work.

  • Pipeline finished with Failed
    5 days ago
    Total: 138s
    #558778
  • πŸ‡¨πŸ‡­Switzerland znerol

    Added a draft MR. This prevents the event dispatcher from being instantiated when a page is delivered from the cache.

  • Pipeline finished with Failed
    5 days ago
    Total: 679s
    #559375
  • πŸ‡¨πŸ‡­Switzerland znerol

    Setting to NR for early feedback. This still needs tests for the deprecation in the page cache middleware. I think we should deprecate the responder tag attribute as well. This could be done in a follow-up though.

  • Pipeline finished with Success
    4 days ago
    Total: 8026s
    #559413
  • πŸ‡¨πŸ‡¦Canada Charlie ChX Negyesi 🍁Canada

    Amazing work, thanks.

    But if this is necessary then what is a lazy service, wasn't that supposed to solve this? And if it doesn't, should the code setting that up be removed?

  • πŸ‡¨πŸ‡­Switzerland znerol

    I do not know what happened to lazy services since #2473113: All stack middlewares are constructed at the same time even for cached pages β†’ . They are being removed gradually anyway, so didn't bother to look.

  • πŸ‡¨πŸ‡­Switzerland znerol

    And if it doesn't, should the code setting that up be removed?

    Yes. In a follow-up.

  • A couple questions on the MR.

  • πŸ‡¨πŸ‡­Switzerland znerol

    Went through an epic git bisect only to find that this broke just two months after it was introduced. Everything happened before 8.0 was even released.

    Lazy middlewares and responder tag introduced in: #2473113-47: All stack middlewares are constructed at the same time even for cached pages β†’ commit date: 2025-04-30

    Lazy middlewares and responder tag broken by: #2408371-83: Proxies of module interfaces don't work β†’ commit date: 2025-06-30

  • πŸ‡¨πŸ‡­Switzerland znerol

    #2408371: Proxies of module interfaces don't work β†’ introduced ProxyServicesPass. But that runs before StackedKernelPass. So the lazy flag added StackedKernelPass to middleware definitions on top of the first responder has no effect.

    I tried to move the StackedKernelPass before ProxyServicesPass.

    diff --git a/core/lib/Drupal/Core/CoreServiceProvider.php b/core/lib/Drupal/Core/CoreServiceProvider.php
    index cd9766c14b5..26cab2964e6 100644
    --- a/core/lib/Drupal/Core/CoreServiceProvider.php
    +++ b/core/lib/Drupal/Core/CoreServiceProvider.php
    @@ -73,14 +73,14 @@ public function register(ContainerBuilder $container) {
    
         $container->addCompilerPass(new SuperUserAccessPolicyPass());
    
    +    $container->addCompilerPass(new StackedKernelPass());
    +
         $container->addCompilerPass(new ProxyServicesPass());
    
         $container->addCompilerPass(new BackendCompilerPass());
    
         $container->addCompilerPass(new CorsCompilerPass());
    
    -    $container->addCompilerPass(new StackedKernelPass());
    -
         $container->addCompilerPass(new StackedSessionHandlerPass());
    
         $container->addCompilerPass(new MainContentRenderersPass());
    

    But ProxyServicesPass doesn't like that:

    [warning] Missing proxy class 'Symfony\Component\ProxyClass\HttpKernel\HttpKernel' for lazy service 'http_kernel.basic'.
    Use the following command to generate the proxy class:
      php core/scripts/generate-proxy-class.php 'Symfony\Component\HttpKernel\HttpKernel' "[namespace_root_path]"
    
    ProxyServicesPass.php:63
    

    Besides the HttpKernel, the following middleware classes are reported:

    • Drupal\Core\ProxyClass\StackMiddleware\ContentLength
    • Drupal\Core\ProxyClass\StackMiddleware\KernelPreHandle
    • Drupal\Core\ProxyClass\StackMiddleware\Session
    • Drupal\big_pipe\ProxyClass\StackMiddleware\ContentLength

    There are for sure more missing in contrib and custom code. Thus, trying to fix the lazyness is not an option I guess.

  • Pipeline finished with Failed
    3 days ago
    Total: 254s
    #560522
  • The Needs Review Queue Bot β†’ tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    I unpublished the cr.

  • πŸ‡¨πŸ‡¦Canada Charlie ChX Negyesi 🍁Canada

    The CR is not clear to me. Is there a before/after of what should be done?

  • πŸ‡¨πŸ‡­Switzerland znerol

    The CR is a stub. Help welcome.

  • Pipeline finished with Failed
    3 days ago
    Total: 435s
    #560613
  • πŸ‡¨πŸ‡­Switzerland znerol
  • Pipeline finished with Failed
    3 days ago
    Total: 455s
    #560643
  • πŸ‡¨πŸ‡­Switzerland znerol
  • πŸ‡¨πŸ‡­Switzerland znerol

    Performance (Drupal core standard install)

    Software:

    % uname -v
    #1 SMP PREEMPT_DYNAMIC Debian 6.1.140-1 (2025-05-22)
    
    % php -v
    PHP 8.3.23 (cli) (built: Jul  3 2025 16:20:45) (NTS)
    Copyright (c) The PHP Group
    Zend Engine v4.3.23, Copyright (c) Zend Technologies
        with Zend OPcache v8.3.23, Copyright (c), by Zend Technologies
        with Xdebug v3.4.4, Copyright (c) 2002-2025, by Derick Rethans
    
    % podman ps
    CONTAINER ID  IMAGE                          COMMAND     CREATED        STATUS            PORTS       NAMES
    2967cbf5773c  docker.io/library/postgres:16  postgres    4 minutes ago  Up 4 minutes ago              drupal_memory
    

    Webserver:

    % export PHP_CLI_SERVER_WORKERS=4
    % php -S localhost:8888
    

    Core baseline:

    % composer create-project --no-interaction "drupal/recommended-project:^11.x-dev" core-dev
    # Install core standard profile
    # Disable css/js aggregation (not compatible with php built-in webbrowser)
    # Add node/1
    # Rebuild container, clear caches, warm cache
    % ab -c 1 -n 500 http://localhost:8888/node/1 > ./core-dev-baseline.txt
    
    Requests per second:    44.61 [#/sec] (mean)
    Time per request:       22.415 [ms] (mean)
    Time per request:       22.415 [ms] (mean, across all concurrent requests)
    

    Core + patch:

    % patch -p1 < /tmp/perf.patch
    % # Rebuild container, clear caches, warm cache
    % ab -c 1 -n 500 http://localhost:8888/node/1 > ./core-dev-patched.txt
    
    Requests per second:    55.61 [#/sec] (mean)
    Time per request:       17.984 [ms] (mean)
    Time per request:       17.984 [ms] (mean, across all concurrent requests)
    
  • πŸ‡¨πŸ‡­Switzerland znerol

    Drupal CMS baseline:

    % composer create-project --no-interaction "drupal/cms:^1.2.x-dev" cms-dev
    # Enable some recipes. I had to disable case management and project management due to field storage issues..
    # Disable css/js aggregation (not compatible with php built-in webbrowser)
    # Rebuild container, clear caches, warm cache
    % ab -c 1 -n 500 http://localhost:8888/ > ./cms-dev-baseline.txt
    
    Requests per second:    29.76 [#/sec] (mean)
    Time per request:       33.597 [ms] (mean)
    Time per request:       33.597 [ms] (mean, across all concurrent requests)
    

    Drupal CMS + patch:

    % patch -p1 < /tmp/perf.patch
    % # Rebuild container, clear caches, warm cache
    % ab -c 1 -n 500 http://localhost:8888/ > ./cms-dev-patched.txt
    
    Requests per second:    44.32 [#/sec] (mean)
    Time per request:       22.563 [ms] (mean)
    Time per request:       22.563 [ms] (mean, across all concurrent requests)
    
  • πŸ‡¨πŸ‡­Switzerland znerol

    (forgot to attach raw data)

  • There are a couple test failures, not sure if intermittent or unrelated.

    One thing to think about, and maybe this is a follow up:
    Deprecate prepending the decorated http_kernel.basic service object directly, so that it's always the service closure prepended as an argument to each http middleware instead. That way we eventually don't need to run reflection and get the constructor arguments at all.

  • πŸ‡¨πŸ‡­Switzerland znerol

    The failure of UncaughtExceptionTest::testLoggerException() is interesting. This test examines the PHP error.log in order to check whether exceptions get logged when the normal logger failed.

    https://git.drupalcode.org/issue/drupal-3538294/-/pipelines/560646/test_...

    Drupal\FunctionalTests\Bootstrap\UncaughtExceptionTest::testLoggerException
    The error + the error that the logging service is broken has been written to the error log.
    Failed asserting that actual size 12 matches expected size 10.
    
    core/tests/Drupal/FunctionalTests/Bootstrap/UncaughtExceptionTest.php:251
    

    I reproduced it locally and found that NegotiationMiddleware appears in the stack trace with the patch, but doesn't on the 11.x branch. The reason is that MonkeysInTheControlRoom middleware is registered with the same priority. But since PriorityTaggedServiceTrait::findAndSortTaggedServices() uses a different method of sorting, the middlewares swapped their location in the stack.

    Bumped the monkey middleware to an appropriate priority to solve the test fail (404).

  • Pipeline finished with Success
    3 days ago
    Total: 674s
    #560849
  • πŸ‡¨πŸ‡­Switzerland znerol
  • πŸ‡¬πŸ‡§United Kingdom catch

    I think the dates in #12 should be 2015 rather than 2026?

    It's a shame we don't currently have a way to cover this with performance tests, but it would end up being almost as specific as the new test coverage, and the explicit test coverage added here looks like it should stop this regressing again. Maybe one day - it should be enough of an improvement to show on the dashboard though which is nice.

    Would a service locator be an option here instead of the lazy services pass? We've been switching to those in πŸ“Œ Replace lazy service proxy generation with service closures Active and similar issues.

  • πŸ‡¨πŸ‡­Switzerland znerol

    I think the dates in #12 should be 2015 rather than 2026?

    Thanks, fixed.

    Would a service locator be an option here instead of the lazy services pass? We've been switching to those in πŸ“Œ Replace lazy service proxy generation with service closures Active : Remove lazy declaration and proxy class for cron and use service closure instead and similar issues.

    I haven't looked into service locators. The cited issue is using service closures (same tech as the PR). Lazy services are not part of the fix (the dysfunctional lazy flagging is removed in this PR).

    As for the question whether it would be possible to do this without a custom service pass: I do not yet know. The service pass has two responsibilities: Assemble every service tagged with http_middleware into a stack. This is done by prepending a reference to the decorated service to the arguments array of the decorator. The second responsibility is to inject all discovered middlewares into the StackedHttpKernel. This is required to propagate the terminate call. Maybe the symfony container provides all the necessary tooling to realize that. But even then, it would be necessary to add some kind of BC pass to move old declarations (tagged with http_middleware to their spot in the stack. I'd be interested to explore. Not sure whether this should be done in this issue or in a follow-up.

Production build 0.71.5 2024