When working on improving render caching of menu blocks, I noticed each entry had the canonical path for the current path as part of its cid resulting in many different variants.

After some digging I discovered this is due to the following snippet in "\Drupal\rules\ContextProvider\CurrentPathContext::getRuntimeContexts":

  Url::fromRoute('<current>', [], ['absolute' => TRUE])->toString();

This gets called early on in the rendering process causing the "\Drupal\Core\RouteProcessor\RouteProcessorCurrent::processOutbound" (https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21RouteProc...)

if ($bubbleable_metadata) {
   $bubbleable_metadata->addCacheContexts(['route']);
}

to bubble up the "route" cache tag. When subsequent blocks get rendered they inadvertently create cache redirects (i.e. variants) in the cache_render table based on the route.

A quick fix would be to handle the bubbleable metadata in the CurrentPathContext class. This can be accomplished by adding the `TRUE` flag to the toString function of Url. In doing so the bubbleable metadata will be returned directly instead of pushed up the chain. See https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Url.php/f...

$values = [
      'path' => $this->currentPathStack->getPath(),
      'url' => Url::fromRoute('<current>', [], ['absolute' => TRUE])->toString(TRUE)->getGeneratedUrl(),
    ];

    $context_definition = new ContextDefinition('current_path', $this->t('Current path'));
    $context = new Context($context_definition, $values);
    $cacheability = new CacheableMetadata();
    $cacheability->setCacheContexts(['url.path']);
    $context->addCacheableDependency($cacheability);

    $result = [
      'current_path' => $context,
    ];

This prevents the 'route' cache tag from bubbling up further. However, I am not sure why we even have the absolute url available in the context? Is this required? Or is the path enough?

CommentFileSizeAuthor
#6 rules-3161036-6-v2.patch3 KBroderik
#6 rules-3161036-6-v1.patch2.2 KBroderik
#6 callchain.png505.96 KBroderik
#5 early-rendering-exception-3161036-5.patch1.58 KBvinay15
#2 rules-3161036-2.patch745 bytesetroid

Issue fork rules-3161036

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:

About issue forks

Comments

Etroid created an issue. See original summary.

etroid’s picture

etroid commented
StatusFileSize
new rules-3161036-2.patch745 bytes

Here's a patch to prevent bubbling from route cache context.

tr’s picture

tr commented

Can you read #3160515: CurrentPathContext runs any time a URL::from() method is invoked and #3160996: 8.x-3.0-alpha6 breaks the search of Search API pages module and see if perhaps those are also symptoms of this problem?

The absolute URL is needed in the context in part because of backwards compatibility - the Entity API in D7 created a 'site' global variable in D7 that was used by Rules, and that 'site' global variable contained the absolute URL among other things. However core Drupal in D8 does not provide this global site information, and neither does the D8 Entity API, so we added all this 'site' information to Rules in D8 as global context variables. One of the main uses of this is to provide token values for email etc (these are typed data tokens, not core Drupal tokens, so in this case {{@rules.current_path_context:current_path.url }})

Although, looking at this again, I'm not sure why the URL is being set to a string. The intent was to use a uri datatype. See #3098291: Current Path datatype and global context.

tr’s picture

tr commented
Status: Active » Postponed (maintainer needs more info)
vinay15’s picture

vinay15
he/him
Primary language Marathi
Location Navi Mumbai
commented
Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new early-rendering-exception-3161036-5.patch1.58 KB

We are facing a similar issue where we have a decoupled setup powered by jsonapi, and we show the decoupled preview of a node on Drupal site using decoupled_router module. We are using rules to purge the cache on decoupled, triggered on node save.

In the recent update of rules (8.x-3.0-alpha5 => 8.x-3.0-alpha6), the decoupled preview links started to break. After some debugging, it was found that the issue is in the recently added `ContextProvider`s.

line 42 of "\Drupal\rules\ContextProvider\CurrentPathContext::getRuntimeContexts":

'url' => Url::fromRoute('<current>', [], ['absolute' => TRUE])->toString(),

line 48 of “\Drupal\rules\ContextProvider\SiteContext::getRuntimeContexts":

'url' => Url::fromRoute('<front>', [], ['absolute' => TRUE])->toString(),

line 49 of “\Drupal\rules\ContextProvider\SiteContext::getRuntimeContexts":

'url' => Url::fromRoute('user.page', [], ['absolute' => TRUE])->toString(),

The fix here is to pass TRUE to `toString(TRUE)` as a parameter, which will collect bubbleable metadata. This fixed our issue, but we haven’t tested its impact on Drupal frontend and would like to discuss if this is the correct way to proceed.

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
StatusFileSize
new callchain.png505.96 KB
new rules-3161036-6-v1.patch2.2 KB
new rules-3161036-6-v2.patch3 KB

This is indeed a major issue. Inadvertently bubbled-up cache contexts are 'polluting' render caches as @Etroid described... if the caller of is actually rendering anything. If it is not rendering anything, Drupal throws a fatal error and the request is terminated. @Vinay15 is also reporting such an error.

This fatal error is very hard to debug for anyone who doesn't know where to start. (If anyone is interested: the best post I've seen so far about adventures in debugging this error is https://www.lullabot.com/articles/early-rendering-a-lesson-in-debugging-... ) The error reported in #3136339: Leaked metadata when using Content Access against the samlauth module is related - it mentions Content Access, but actually it's the Rules code in this issue that 'leaks' metadata and causes the exception.

Similar call chain as Etroid must have seen, same cause, but different effect.

@ TR / #3:

Can you read #3160515: CurrentPathContext runs any time a URL::from() method is invoked and #3160996: 8.x-3.0-alpha6 breaks the search of Search API pages module and see if perhaps those are also symptoms of this problem?

They are not; they require different changes (to the same code) - see #3160515-35: CurrentPathContext runs any time a URL::from() method is invoked.
EDIT: To be more exact: the error reported in #3136339 / the above screenshot can be solved by both. I suspect this is the same for @Vinay15's problem. 'My' fatal crash can be solved both by fixing the Url::toString() call (this issue), and by making sure getAvailableContexts() does not make Url::toString() calls anymore (#3160515-37 patch). However, after solving #3160515, the Url::toString() call still pollutes the render cache by creating too many variants.

For this cache context issue, the basic rules (pieced together by exploring various links) are:

  • any Drupal code calling Url::toString() MUST think about whether the URL it outputs, is always part of some renderable content/response that will be cached / is executing inside a valid 'render context'.
  • (If the answer is "yes, always", then... calling toString() is fine; toString() will magically take care that the 'bubbleable cache metadata' gets bubbled up to the right place / stuff is cached in the right way.)
  • If the answer is "no, not always", then the caller MUST call toString(TRUE) which circumvents the 'magic' (that can cause exceptions), and decide what to do with the cache metadata that is passed back by toString(TRUE)

So the patch is definitely going in the right direction. But I have a question... because I know Url issues, but I only have very basic knowledge about Plugins and no experience with ContextProviders.

I see getRuntimeContexts() is returning Plugin Contexts. The info in your Plugin Contexts contains URL strings... which are going to be different strings, depending on certain cache contexts (e.g. language). My question is: should those differences (the related difference in cache contexts/metadata) be propagated into the returned Plugin Contexts too?

  • If not (because we don't care what the exact URL string is, because $reason)... then the patch is good. I combined patches from @Etroid and @Vinay15 into 'variant 1'. (@Vinay15 / #5: thank you for finding extra occurrences of toString(). But Url::toString(TRUE) is tricky: this returns a GeneratedUrl object instead of a string. So we need to do Url::toString(TRUE)->getGeneratedUrl() instead. Yes, it's a DrupalWTF - which will hopefully get fixed someday.)
  • If yes, then we need to pick the cache contexts out of the GeneratedUrl object. Patch variant 2 does this. Note this should still fix the original reported issue: the toString(TRUE) calls take care that the URLs' bubbleable/cache metadata doesn't get pushed into the render context which @Etroid sees being polluted. The difference is that the metadata gets handled/returned, instead of discarded. (A note: I'm fairly sure that in CurrentPathContext, the merge() call could be optimized away because the URL metadata/GeneratedUrl object always already contains the 'path.url' context. But I don't want to think about it.)

Both patches have another addition: the toString() call in RulesDebugLoggerChannel is another danger for the fatal error (or polluted render context), so the same change is made there. The bubbleable/cache metadata just gets discarded - I'm pretty sure we don't care about it there.

I'm sure this patch will conflict with #3160515: CurrentPathContext runs any time a URL::from() method is invoked; one or the other will need to get re-rolled.

Status: Needs review » Needs work

The last submitted patch, 6: rules-3161036-6-v2.patch, failed testing. View results

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Status: Needs work » Needs review

Hm. Does anyone have experience with what's happening with the failing assertValidTokens($cache_contexts) in those tests? I'm sure my change made that happen (because I'm changing the cache contexts returned inside the plugin contexts) but I have no idea how to check what, where.

Or maybe it doesn't matter, if the answer to the question I asked is "no (because we don't care what the exact URL string is, because $reason)". Which means variant 1 is the right patch.

lukus’s picture

lukus
he/him
commented

Hi - we're experiencing the same problem indicated here.

Applying the patch (https://www.drupal.org/files/issues/2021-01-02/rules-3161036-6-v1.patch) resolves the immediate problem (as we don't care about URL variants needing specific cache contexts), but as it sounds like the best route still needs to be fully defined, wondering what the next steps are?

wombatbuddy’s picture

wombatbuddy commented

We had the problem with the 'View Mode Page' module (after visit of the page with a custom display, the similar error message appears).
The patch #6 solved this problem.

wombatbuddy’s picture

wombatbuddy commented
Status: Needs review » Reviewed & tested by the community
tbsiqueira’s picture

tbsiqueira
He/him
commented

We also tested patch #6 and it resolved the caching issue, for users that had access to menu the pageload dropped from 6 seconds to 1 second.

tr’s picture

tr commented
Version: 8.x-3.x-dev » 4.0.x-dev
chrisolof’s picture

chrisolof
he/him
Primary language English
commented

This bug is a bad one for performance. On our site it was behind a very problematic "route" cache context bubbling onto many of our site's render arrays, causing lots of unnecessary re-rendering per route and a pile-up of identical render cache entries (again, per route). This has the effect of largely disabling the benefits of render caching, while at the same time ballooning render cache write operations and render cache storage needs.

I can confirm that rules-3161036-6-v2.patch from #6 solves the issue, and seems to do so in a way that preserves cacheability metadata from URL generation where needed without letting it bubble onto unrelated render arrays. It doesn't seem like it would be wise to just totally toss this metadata out, as the v1 patch does.

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

fago opened merge request !103

fago’s picture

fago
Primary language German
Location Vienna
commented

The suggest patch/fix makes a lot of sense. I'd be happy to merge, but we need to run the test pipeline first and verify it causes no regressions. Added MR based upon roderik's lattest/3 patch.

fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Reviewed & tested by the community » Needs work

It seems the changes require some test-changes also. Thus this needs works.