Contrib.social

TTL handling broken, always permanent

Open on Drupal.org →
Created on 29 October 2020, about 4 years ago
Updated 21 February 2023, over 1 year ago

Problem/Motivation

  protected function getExpiration($expire) {
    if ($expire == Cache::PERMANENT || $expire > $this->permTtl) {
      return $this->permTtl;
    }
    return $expire - \Drupal::time()->getRequestTime();
  }

This looks perfectly reasonable but there is a subtle bug because it will basically always hit the permanent case and never return the calculated value.

First, permTtl is equal to LIFETIME_PERM_DEFAULT(31536000) unless otherwise set through config. So when you say set the cache to say 600 the call ends up looking something like $this->getExpiration(\Drupal::time()->getRequestTime() + 600); which we would expect to return the 600. However, the value will look something like this 1604003270 which is always larger than which is the default value and the TTL sent to redis will be the permanent TTL.

Steps to reproduce

Set a cache entry to a couple minutes in the future and check the ttl in redis. You'll see something close to the PERM ttl instead of the provided ttl.

Proposed resolution

Fix the default permTtl setup to take into account the request time.

Remaining tasks

User interface changes

n/a

API changes

maybe? this seems like a straight up bug but its possible someone was relying on the the permTtl value somehow? Its protected so I think its ok to correct the logic.

Data model changes

n/a

🐛 Bug report
Status

Needs work

Version

1.0

Component

Code

Created by

🇺🇸United States neclimdul Houston, TX

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Comment over 1 year ago →
    🇨🇭Switzerland berdir Switzerland

    This module now has tests on merge requests ( doesn't show up here), and they are failing, which is expected based on what was discussed in the related github issue.

  • Comment over 1 year ago →
    🇺🇸United States lcatlett

    This patch has been required for Pantheon customers hitting maxmemory limit and is critical to evicting keys as expected using LRU

  • First commit to issue fork.
  • Merge request !30Resolve #3179757 "Ttl handling broken testing" → (Open) created by Ec1ipsis
  • Pipeline finished with Failed
    8 months ago
    Total: 253s
    #125516
  • Pipeline finished with Failed
    8 months ago
    Total: 195s
    #126369
  • Pipeline finished with Failed
    8 months ago
    Total: 219s
    #128762
  • Pipeline finished with Failed
    8 months ago
    Total: 317s
    #128772
  • Pipeline finished with Success
    8 months ago
    Total: 202s
    #130874
  • Pipeline finished with Failed
    8 months ago
    Total: 253s
    #131928
  • Pipeline finished with Failed
    8 months ago
    Total: 195s
    #131933
  • Pipeline finished with Success
    8 months ago
    Total: 312s
    #131939
  • Comment 8 months ago →
    🇺🇸United States Ec1ipsis

    I can't tell whether the automated test needs to change or if the behavior itself has a bug. I've merged in the most recent release to the 8.1 branch, and the error that I'm seeing from PHPUnit is:

    1) Drupal\Tests\redis\Kernel\RedisCacheTest::testSetGet
Failed asserting that false is of type "object".
/builds/issue/redis-3179757/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122

    I believe that the automated test itself needs to be updated, but I'm not certain. Anyone have a firm answer on that?

  • Comment 8 months ago →
    🇨🇭Switzerland berdir Switzerland

    See the duplicate issue #2877893: Cache Expiration doesn't follow Drupal conventions → where I've explained this about 7 years ago :)

    The test is from core. Core expects that the API allows to return expired cache items if you ask for that. Setting a TTL in redis is incompatible with the expectation that core has for a cache backend implementation.

    The only way to get tests to pass is to make this behavior configurable. Either a boolean enable/disable, or an idea I just had, is that we add a ttl offset setting. So that can be set to e.g. 300, then any ttl we receive is extended by 300s, which means that for this timeframe, we can still return expired items, allowing certain use cases like returning expired items while one process recalculates it. Then we just need to configure that specific test to have enough ttl offset for the tests to still work.

  • Pipeline finished with Failed
    8 months ago
    Total: 250s
    #134637
  • Pipeline finished with Failed
    8 months ago
    Total: 200s
    #135660
  • Pipeline finished with Failed
    8 months ago
    Total: 271s
    #135667
  • Pipeline finished with Failed
    8 months ago
    Total: 209s
    #135676
  • Pipeline finished with Failed
    8 months ago
    Total: 189s
    #137794
  • Pipeline finished with Failed
    8 months ago
    Total: 180s
    #137861
  • Pipeline finished with Failed
    8 months ago
    Total: 178s
    #137873
  • Pipeline finished with Failed
    8 months ago
    Total: 229s
    #137955
  • Pipeline finished with Failed
    8 months ago
    Total: 225s
    #137980
  • Pipeline finished with Failed
    8 months ago
    Total: 217s
    #138006
  • Pipeline finished with Failed
    8 months ago
    Total: 194s
    #138034
  • Pipeline finished with Failed
    8 months ago
    Total: 225s
    #138043
  • Comment 6 months ago →
    🇹🇷Turkey kyilmaz80

    Any updates?

  • Comment about 1 month ago →
    🇧🇷Brazil jribeiro Campinas - São Paulo

    Rerolled patch for 1.8 version of the module without the change to the line: 

    $this->permTtl = ($iv->y * 31536000 + $iv->m * 2592000 + $iv->days * 86400 + $iv->h * 3600 + $iv->i * 60 + $iv->s);

    This line is conflicting with the issue/patch https://www.drupal.org/node/2944938 → , since we are using both patches on our projects.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024