ApcuBackend::set() passes the $expire (timestamp) directly as the $ttl (duration) to apc_store().

ApcuBackend::invalidate*() does not properly preserve cached data, tags and checksums by using ::set() instead of apc_store().

ApcuBackend::$tagCache is not effective because entries are stored with the tags prefix but read without it.

PhpBackend::deleteTags() does not remove invalid items #2267643: Cache\PhpBackend::deleteTags() does not remove invalid items

CommentFileSizeAuthor
#43 2272685-nr-bot.txt1.53 KBneeds-review-queue-bot
#36 2272685.36.patch3.71 KBberdir
#23 2272685.23.patch4.06 KBalexpott
#23 22-23-interdiff.txt2.44 KBalexpott
#22 2272685-22.patch3.22 KBolli
#20 interdiff.txt1.65 KBolli
#20 2272685-20.patch3.21 KBolli
#17 interdiff.txt3.35 KBolli
#17 2272685-17.patch7.1 KBolli
#12 interdiff.txt1.86 KBolli
#12 2272685-12.patch7.18 KBolli
#8 interdiff.txt4.63 KBolli
#8 2272685-8.patch5.66 KBolli
#8 2272685-test_only.patch2.18 KBolli
#3 2272685-3.patch4 KBolli
#3 2272685-test_only.patch1.22 KBolli
apcu.patch4.06 KBolli

Comments

Status: Needs review » Needs work

The last submitted patch, apcu.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review

apcu.patch queued for re-testing.

olli’s picture

olli commented
StatusFileSize
new 2272685-test_only.patch1.22 KB
new 2272685-3.patch4 KB

Reroll for psr-4 and a test only patch.

The last submitted patch, 3: 2272685-test_only.patch, failed testing.

andypost queued 3: 2272685-3.patch for re-testing.

dawehner’s picture

dawehner
Primary language German
commented 
  1. +++ b/core/lib/Drupal/Core/Cache/ApcuBackend.php
@@ -204,9 +204,12 @@ public function set($cid, $data, $expire = CacheBackendInterface::CACHE_PERMANEN
+      $ttl = 0;

    Can we use a non-short variable name? 

  2. +++ b/core/lib/Drupal/Core/Cache/ApcuBackend.php
@@ -281,8 +285,8 @@ public function invalidateMultiple(array $cids) {
     foreach ($this->getAll() as $data) {
-      $cid = str_replace($this->binPrefix, '', $data['key']);
-      $this->set($cid, $data['value'], REQUEST_TIME - 1);
+      $data['value']->expire = REQUEST_TIME - 1;
+      apc_store($data['key'], $data['value'], 1);
     }
   }

    Not sure whether I am convinced of using apc_store not $this->set tbh.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

$ttl is fine as variable name (see TTL).

2. sounds minor to me?

@olli: Any chance you could revise the issue title + summary to better explain what this issue/patch is actually about? :)

olli’s picture

olli commented
Issue summary: View changes
StatusFileSize
new 2272685-test_only.patch2.18 KB
new 2272685-8.patch5.66 KB
new interdiff.txt4.63 KB

Thanks for the reviews. Can't use set there because that would recalculate the deletions checksum. I've added coverage for this. The ::invalidateMultiple() was wrong using getMultiple/apc_store, fixed by replacing getMultiple with apc_fetch.

Status: Needs review » Needs work

The last submitted patch, 8: 2272685-8.patch, failed testing.

The last submitted patch, 8: 2272685-test_only.patch, failed testing.

olli’s picture

olli commented
Related issues: +#2267643: Cache\PhpBackend::deleteTags() does not remove invalid items
olli’s picture

olli commented
Status: Needs work » Needs review
StatusFileSize
new 2272685-12.patch7.18 KB
new interdiff.txt1.86 KB

Merged #2267643: Cache\PhpBackend::deleteTags() does not remove invalid items

sun’s picture

sun
Primary language German
Location Karlsruhe
commented 
+++ b/core/lib/Drupal/Core/Cache/ApcuBackend.php
@@ -236,7 +239,7 @@ public function deleteMultiple(array $cids) {
   public function deleteAll() {
-    apc_delete(new \APCIterator('user', '/^' . preg_quote($this->binPrefix, '/') . '/'));
+    apc_delete($this->getAll());
   }

Hm. AFAIK, APCIterator does not immediately fetch all data from the store, it only fetches when iterating over the items.

By passing the iterator directly to apc_delete(), all currently stored items are not fetched, but directly deleted.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Sorry, please ignore #13 — getAll() returns an APCIterator.

olli’s picture

olli commented

We could rename getAll() to getIterator() or something.

olli’s picture

olli commented
Title: APCu follow-up » Improve handling of invalid cache items in apcu and php backends
Issue summary: View changes 
+++ b/core/lib/Drupal/Core/Cache/ApcuBackend.php
@@ -271,8 +274,9 @@ public function invalidate($cid) {
+    foreach (apc_fetch(array_map(array($this, 'getApcuKey'), $cids)) as $key => $cache) {

I think we need to check the return value of apc_fetch() here too.

olli’s picture

olli commented
Title: Improve handling of invalid cache items in apcu and php backends » Incorrect handling of invalid cache items in apcu and php backends
StatusFileSize
new 2272685-17.patch7.1 KB
new interdiff.txt3.35 KB

Reroll and a check for the return value of apc_fetch (like in #2332427: APCUBackend makes incorrect assumptions about apc_fetch return type.).

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I think #918538: Decouple cache tags from cache bins means that a lot here will no longer be needed? Btw, would be great if you review that, you seem to be finding a lot of bugs in the cache backends :)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

That landed, so this needs a reroll

olli’s picture

olli commented
Issue tags: -Needs reroll
StatusFileSize
new 2272685-20.patch3.21 KB
new interdiff.txt1.65 KB

Here's a reroll.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
olli’s picture

olli commented
Title: Incorrect handling of invalid cache items in apcu and php backends » Incorrect handling of invalid cache items in apcu backend
StatusFileSize
new 2272685-22.patch3.22 KB

reroll

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 22-23-interdiff.txt2.44 KB
new 2272685.23.patch4.06 KB

I think we shouldn't we be setting the ttl time on APC cache entries. We should leave this for the user to configure in their APC settings. The idea of setting ttl's for invalidations seems wrong too because invalidations are handled by the expire time set on the cache object. The default ttl for user cache objects is 0 - which means they are permanent.

Also the whole point of invalidations is so we can use $allow_invalid so setting the ttl to 1 breaks this.

The only reason this is not a major bug is because we have no usages of $allow_invalid in core.

Garbage collection and APC is also interesting since many people run cron using cron and therefore this process will not have access to the APC cache used by the web server. This suggests that the whole tactic of allow invalid might actually not be sensible for an APC backend.

Status: Needs review » Needs work

The last submitted patch, 23: 2272685.23.patch, failed testing.

Status: Needs work » Needs review

jhedstrom queued 23: 2272685.23.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 23: 2272685.23.patch, failed testing.

fabianx’s picture

fabianx commented

I don't think ACPu needs garbage collection, but yes allow_invalid is incompatible with that and need to check timestamp ourselves there - the same as we check the tags.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

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

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

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

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

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

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

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

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

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

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

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

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

Drupal 8.6.x will not receive any further development aside from security fixes. 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.

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.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new 2272685.36.patch3.71 KB

*Berdir casts a reroll spell, puts on his +2 git skills gloves and rolls the dice...*

Found this due to #2575105: Use cache collector for state, we actually do have a usage of cache invalidation in core, just not one using fast chained/apcu I suppose. Until that state patch, that is.

andypost’s picture

andypost
he/him
Primary language Russian
commented

Great to see tests!

+++ b/core/lib/Drupal/Core/Cache/ApcuBackend.php
@@ -206,21 +208,25 @@ public function deleteMultiple(array $cids) {
-    apcu_delete($this->getIterator('/^' . preg_quote($this->binPrefix, '/') . '/'));
+    $this->removeBin();
...
+    foreach ($this->getAll() as $data) {
...
-    apcu_delete($this->getIterator('/^' . preg_quote($this->binPrefix, '/') . '/'));
+    apcu_delete($this->getAll());

getAll() using different logic to implement prefix, but looks that's fine...

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

This seems to cause a massive performance issue as the test is aborted after 110min, is that the garbage collection?

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

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

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

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

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should 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.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 2272685-nr-bot.txt1.53 KB

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

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

Version: 9.5.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. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

catch’s picture

catch
he/him
Primary language English
commented
Related issues: +#3586418: Set explicit ttl in the apcu backend and allow a default to be set for chained fast

Some crossover here with #3586418: Set explicit ttl in the apcu backend and allow a default to be set for chained fast although that issue is setting artificially short TTLs to avoid having any kind of explicit garbage collection step.