Problem/Motivation

First requests to the httpKernel will cache request properly but the following requests handled by the same httpKernel instance will return the same response that was cached for the first request.

Proposed resolution

Add check for the comparison of cached cid and current cid in the getCacheId() method of the Drupal\page_cache\StackMiddleware\PageCache class.

Remaining tasks

-

User interface changes

-

API changes

-

Data model changes

-

Release notes snippet

-

Issue fork drupal-3050383

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

voleger created an issue. See original summary.

voleger’s picture

voleger’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: 3050383-02.patch, failed testing. View results

sames’s picture

This problem persists in Drupal 8.7.0. I am glad I found the patch as it fixes the problem for me. Most people affected are probably using the subrequests contrib module.

Multiple workarounds to be found here:
https://www.drupal.org/project/subrequests/issues/3049395

waspper’s picture

I've experienced same issue. Proposed patch seems to solve issue.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.9 was released on November 6 and is the final full bugfix release for the Drupal 8.7.x series. Drupal 8.7.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.8.0 on December 4, 2019. (Drupal 8.8.0-beta1 is available for testing.)

Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

jhedstrom’s picture

Issue tags: +Needs tests

This will need tests, and to address the failing tests from the patch above.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

mxr576’s picture

Version: 8.9.x-dev » 9.2.x-dev
anmolgoyal74’s picture

Status: Needs work » Needs review
FileSize
1.41 KB
653 bytes

Slightly update the condition. Checking with 9.2.x

Status: Needs review » Needs work

The last submitted patch, 11: 3050383-11.patch, failed testing. View results

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Lukas von Blarer’s picture

I am also coming having this issue: #3061907: Page Cache duplicating responses for subrequests . But in my case dynamic_page_cache was causing this issue. Do we have the same bug there as well? Is there an issue regarding that already?

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

phenaproxima made their first commit to this issue’s fork.

phenaproxima’s picture

phenaproxima’s picture

The only situation in which I've encountered this problem is in the Subrequests contrib module. See #3049395: Page Cache causes different subrequests to return the same responses.

In general, you wouldn't run into this problem, but Subrequests disguises its subrequests as master requests, specifically in order to allow page caching to kick in. Given that the Subrequests module's purpose is to be a driver for decoupled applications, that makes sense. In theory, Subrequests should just mark its subrequests as subrequests...but then page caching ceases to work for it, and its performance suffers dramatically.

By storing the page cache ID for a given request as an attribute of the request object, we can improve the situation for everyone, non-disruptively. However, I'd consider it a feature because core, from its perspective, is working as intended, and Subrequests can also work around the issue by subclassing the PageCache middleware.

Therefore, I'm re-categorizing this as a feature request and a contrib soft blocker.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

voleger’s picture

Assigned: voleger » Unassigned
Issue tags: +Needs reroll

MR needs to rebase against 10.1.x branch

Bhanu951’s picture

Assigned: Unassigned » Bhanu951
voleger’s picture

@Bhanu951 please revert the merge commit, and do rebase the forked branch over 10.1.x. branch. Merge request has to show only a single commit `Store the page cache ID on the request` as a difference between branch 10.1.x

realityloop made their first commit to this issue’s fork.

realityloop’s picture

Status: Needs work » Needs review

@volegar I've cleaned this branch up to 10.1.x and recommitted the change with attribution to yourself and phenaproxima

catch’s picture

Status: Needs review » Needs work

Left a couple of comments on the MR.

Bhanu951’s picture

Assigned: Bhanu951 » Unassigned

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mglaman’s picture

I think this should be using an SqlObjectStorage object for storing cache IDs per-request instead of request attributes. This avoids page_cache from manipulating the request. Just like \Drupal\Core\Path\CurrentPathStack

Copy of some work I'm adding to

  protected function getCacheId(Request $request) {
    if ($this->cacheIds === NULL) {
      $this->cacheIds = new \SplObjectStorage();
    }

    if (!isset($this->cacheIds[$request])) {
      $cid_parts = [
        $request->getSchemeAndHttpHost() . $request->getRequestUri(),
        $request->getRequestFormat(NULL),
      ];
      $this->cacheIds[$request] = implode(':', $cid_parts);
    }

    return $this->cacheIds[$request];
  }

It doesn't matter what kind of request it is then. And may help solve issues if running Drupal in an event loop runtime.

See https://git.drupalcode.org/project/subrequests/-/merge_requests/9/diffs?... for an example.

bradjones1’s picture

mglaman changed the visibility of the branch 11.x to hidden.

mglaman’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -Needs reroll

I've created a new MR https://git.drupalcode.org/project/drupal/-/merge_requests/5745 that allows multiple main requests using the SplObjectStorage pattern, and a test. First commit showed test failure, second commit includes and shows the fix.

mglaman’s picture

Title: PageCache getCacheId doesn't compare cid of the following subrequests in subrequest queue calls. » PageCache cannot handle multiple main requests

I tried to retitle to something more specific to the problem.

smustgrave’s picture

Status: Needs review » Needs work

Appears to be 1 open thread.

mglaman’s picture

How should we handle the open thread. I don't agree with @catch that we need a comment about why we're supporting multiple cache IDs, when now PageCache is acting as a per-request reverse proxy instead of main-request-only reverse proxy.