Skip to content

Commit

Permalink
Set cache_expiration_time when storing data to cache
Browse files Browse the repository at this point in the history
When we switched to PSR-16 inspired `DataCache` in bb38d74, we introduced a regression due to impedance mismatch between PSR-16 and HTTP Caching mechanisms:
- HTTP caching uses two step invalidation – if a cache entry is accessed after its TTL passes, a request is made with a stored HTTP caching header (e.g. ETag) and the cache is considered invalid only if the server returns something else than 304 Not Modified response.
- On the other hand, PSR-16 cache does not allow access to entries past its TTL, forcing us to fetch the content every time, even when the remote server could tell us it is still fine.

To work around this, bb38d74 decided to store `cache_expiration_time` field in the cache entry so that we could bypass the PSR-16’s strict TTL and handle expiration ourselves. But there were few places where we forgot to set it. This patch fixes that.

Unfortunately, because we also pass the same value as `cache_expiration_time` to `DataCache::set_data` as TTL, the early eviction still happens. We will also need to set the TTL to `null` (unlimited) so that the `DataCache` does not evict the entries behind our back.

Partly fixes simplepie#830 ([BUG] Cache constantly updating)
  • Loading branch information
Alkarex authored Sep 29, 2024
1 parent 6b97dec commit 194f8ff
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 4 deletions.
4 changes: 3 additions & 1 deletion src/SimplePie.php
Original file line number Diff line number Diff line change
Expand Up @@ -1900,7 +1900,7 @@ protected function fetch_data(&$cache)
$this->data = [];
}
// Check if the cache has been updated
elseif (isset($this->data['cache_expiration_time']) && $this->data['cache_expiration_time'] > time()) {
elseif (!isset($this->data['cache_expiration_time']) || $this->data['cache_expiration_time'] < time()) {
// Want to know if we tried to send last-modified and/or etag headers
// when requesting this file. (Note that it's up to the file to
// support this, but we don't always send the headers either.)
Expand All @@ -1924,6 +1924,7 @@ protected function fetch_data(&$cache)
$this->status_code = 0;

if ($this->force_cache_fallback) {
$this->data['cache_expiration_time'] = $this->cache_duration + time();
$cache->set_data($cacheKey, $this->data, $this->cache_duration);

return true;
Expand All @@ -1936,6 +1937,7 @@ protected function fetch_data(&$cache)
// Set raw_data to false here too, to signify that the cache
// is still valid.
$this->raw_data = false;
$this->data['cache_expiration_time'] = $this->cache_duration + time();
$cache->set_data($cacheKey, $this->data, $this->cache_duration);

return true;
Expand Down
6 changes: 3 additions & 3 deletions tests/Integration/CachingTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ public function testInitWithDifferentCacheStateCallsCacheCorrectly(
*/
public static function provideSavedCacheData(): array
{
$defaultMtime = time();
$defaultMtime = time() - 1; // -1 to account for tests running within the same second
$defaultExpirationTime = $defaultMtime + 3600;

$expectDefaultDataWritten = [
Expand Down Expand Up @@ -246,10 +246,10 @@ public static function provideSavedCacheData(): array
[CacheInterface::class, $currentlyCachedDataWithNonFeedUrl, $expectDataWithNewFeedUrl, $defaultMtime],
// Check if the cache has been updated
[Base::class, $currentlyCachedDataIsUpdated, $expectDefaultDataWritten, $defaultMtime],
[CacheInterface::class, $currentlyCachedDataIsUpdated, $expectDefaultDataWritten, $defaultMtime],
[CacheInterface::class, $currentlyCachedDataIsUpdated, $expectNoDataWritten, $defaultMtime],
// If the cache is still valid, just return true
[Base::class, $currentlyCachedDataIsValid, $expectDefaultDataWritten, $defaultMtime],
[CacheInterface::class, $currentlyCachedDataIsValid, $expectNoDataWritten, $defaultMtime],
[CacheInterface::class, $currentlyCachedDataIsValid, $expectDefaultDataWritten, $defaultMtime],
];
}
}

0 comments on commit 194f8ff

Please sign in to comment.