Problem/Motivation

Avoid writing invalid data to the fast backend.

Only relevant if/when someone calls getMultiple() with invalid == TRUE.

Marking issue NOVICE ONLY for creating a patch out of the below code.

Steps to reproduce

- Call getMultiple() with $allow_invalid = TRUE.
- See that fastBackend is happily setting invalid data, which will never be useful

Proposed resolution

<code>
diff --git a/web/core/lib/Drupal/Core/Cache/ChainedFastBackend.php b/web/core/lib/Drupal/Core/Cache/ChainedFastBackend.php
--- a/web/core/lib/Drupal/Core/Cache/ChainedFastBackend.php
+++ b/web/core/lib/Drupal/Core/Cache/ChainedFastBackend.php
@@ -166,7 +166,10 @@ public function getMultiple(&$cids, $allow_invalid = FALSE) {
         $cache[$item->cid] = $item;
         // Don't write the cache tags to the fast backend as any cache tag
         // invalidation results in an invalidation of the whole fast backend.
-        $this->fastBackend->set($item->cid, $item->data, $item->expire);
+        if (!$allow_invalid || $item->valid) {
+          $this->fastBackend->set($item->cid, $item->data, $item->expire);
+        }
       }
     }

Remaining tasks

Review

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

CommentFileSizeAuthor
#3 invalid-data-3395212-1.patch742 bytesdeborahblessy

Issue fork drupal-3395212

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

Fabianx created an issue. See original summary.

deborahblessy’s picture

deborahblessy commented

Iam working on this.

deborahblessy’s picture

deborahblessy commented
StatusFileSize
new invalid-data-3395212-1.patch742 bytes

Hi @Fabianx,

I have created and attached the patch. Could you please verify this?

deborahblessy’s picture

deborahblessy commented
Status: Active » Needs review
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: -Novice +DrupalCon Lille 2023, +Needs tests

Thank you, @deborahblessy!

Next step: test coverage.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

For #5

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » fabianx
Priority: Normal » Major
Status: Needs work » Needs review
Issue tags: +scalability

Catching up on long-forgotten tabs … from last year’s European DrupalCon just before this year’s starts 😄

I think Fabian is best suited to determine what is next here.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Brought this up in #contribute in slack https://drupal.slack.com/archives/C1BMUQ9U6/p1727876410850979

@fabianx agreed on the approach but mentioned still needs tests.

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

nicxvan opened merge request !9730

nicxvan’s picture

nicxvan commented

Moved to MR so tests can run and be more easily written.

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

murilohp’s picture

murilohp commented
Status: Needs work » Needs review
Issue tags: -Needs tests

Hey, just created a new test to validate the proposed solution, moving this to NR and removing the tag for now, thanks!

smustgrave’s picture

smustgrave commented
Assigned: fabianx » Unassigned
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative 
1) Drupal\Tests\Core\Cache\ChainedFastBackendTest::testSetInvalidDataFastBackend
Invalid data was not saved on the fast cache.
stdClass Object (...) does not match expected type "NULL".
/builds/issue/drupal-3395212/core/tests/Drupal/Tests/Core/Cache/ChainedFastBackendTest.php:103
FAILURES!
Tests: 3, Assertions: 7, Failures: 1.
Exiting with EXIT_CODE=1

Shows test coverage.

Not sure if this needs any kind of release note (assuming not)

Looking at the change to core/lib/Drupal/Core/Cache/ChainedFastBackend.php and don't see anything glaringly wrong. Think adding the condition makes sense.

believe this one is good.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

Gave the IS some thought and at first felt like this was wrong:

See that fastBackend is happily setting invalid data, which will never be useful

After all, if you're getting the data while note caring about its validity, you may as well store that in memory for future gets which do not care about validity. However, then I realized that a cache get which allows invalid could the poison the memory cache for a subsequent cache get which does not allow invalid. And that would be really bad.

I still have the eerie feeling that there are edge cases we're not seeing here, but for now I'd say it makes more sense to commit this change rather than keep the old behavior.

So RTBC +1.

  • catch committed 91e01793 on 10.4.x 
    Issue #3395212 by nicxvan, deborahblessy, murilohp, smustgrave, fabianx...

  • catch committed b8c4c25f on 10.5.x 
    Issue #3395212 by nicxvan, deborahblessy, murilohp, smustgrave, fabianx...

  • catch committed 6f4f3a88 on 11.1.x 
    Issue #3395212 by nicxvan, deborahblessy, murilohp, smustgrave, fabianx...

  • catch committed e6b0b85e on 11.x 
    Issue #3395212 by nicxvan, deborahblessy, murilohp, smustgrave, fabianx...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 10.4.x-dev
Status: Reviewed & tested by the community » Fixed

I think this is worth skipping just from a cache efficiency point of view if nothing else - allow_invalid is most likely to be used in the case that stale content is served and then a cache refresh job is queued (or done end of request or similar), so there'll be a new cache write pretty fast after this gets set anyway. Cache poisoning seems theoretical because the fast backend should handle invalid items correctly, but it doesn't hurt.

Committed/pushed to 11.x/11.1.x/10.5.x/10.4.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

nicxvan closed merge request !9730