PageCache getCacheId doesn't compare cid of the following subrequests in subrequest queue calls.

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

Left a couple of comments on the MR.

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.

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

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.

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

Appears to be 1 open thread.

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.

Pushing back to NR to get a discussion on #37.

Could a small comment be added? Then can remark it

I'm not too happy keeping an unbound amount of state inside the page cache middleware. The original approach in #3029373: PageCache::getCacheId() sometimes uses different cache IDs during get and set, causing unnecessary cache misses for some requests → would be compatible with n main requests (look at the patches before comment #13.