Stop legacy file cache constantly updating by Art4 · Pull Request #846 · simplepie/simplepie

Art4 · 2023-08-28T07:41:53Z

This PR fixes a bug introduced in #752.

Fixes #830.

This reverts commit 5c871cc.

Art4 · 2023-08-28T08:18:28Z

Something very strange is going on here. Builds 606 and 608 are for the exact same code changes but the results for different PHP versions is different.

I can not reproduce this on my machine (tests are successfully on PHP 8.2). I assume some race conditions on the Github Actions?

Art4 · 2023-08-28T12:27:37Z

I can reproduce this strange behavior on my GitHub fork: I have rerun the job for the same commit 4 times and got randomly different errors each time.

jtojnar · 2023-09-04T06:13:40Z

I can reproduce it locally. Will try to debug it during the week.

…pdating

simplepie#846

Alkarex · 2024-09-14T13:55:49Z

src/SimplePie.php

                }
                // 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()) {


Suggested change

elseif (isset($this->data['cache_expiration_time']) && $this->data['cache_expiration_time'] < time()) {

elseif (empty($this->data['cache_expiration_time']) || $this->data['cache_expiration_time'] < time()) {

Otherwise you hit the next else which believes the cache to be still valid

Note that the equality here gives problem when tests are running within the same second, which is the reason for your tests failing.

simplepie/tests/Integration/CachingTest.php

Lines 123 to 124 in 9385c54

$defaultMtime = time();

$defaultExpirationTime = $defaultMtime + 3600;

You can for instance change to $defaultMtime = time() - 1;

See FreshRSS#23

Replacement PR with additional fixes #883
Feel free to merge in this PR or in master as you prefer

Otherwise you hit the next else which believes the cache to be still valid

I would expect that when no expiration time is provided, it cannot really expire so this is correct IMO. Though it should not actually happen if I read the code correctly so the isset is there mostly for PHPStan.

If cache_expiration_time is not set for any reason, I would want my cache to be revalidated. Otherwise it can stay like that for a long time

Fair enough. I still wonder why it duplicates the ttl handling, though.

* Merge SimplePie fix 846 simplepie#846 * Fix tests running in the same second

fix simplepie#830 Replaces simplepie#846

jtojnar · 2024-09-25T23:49:36Z

tests/Integration/CachingTest.php

            // Check if the cache has been updated
            [Base::class,           $currentlyCachedDataIsUpdated,          $expectDefaultDataWritten, $defaultMtime],
-            [CacheInterface::class, $currentlyCachedDataIsUpdated,          $expectDefaultDataWritten, $defaultMtime],
+            [CacheInterface::class, $currentlyCachedDataIsUpdated,          $expectNoDataWritten,      $defaultMtime],


Any idea why this differs from the legacy backend?

…pdating

jtojnar · 2024-09-26T08:06:59Z

src/SimplePie.php

@Art4 Why in bb38d74 did you start to set cache_expiration_time as part of the data in the first place? Is it to handle implementations that do not support TTL?

PSR 16 is inconsistent whether that is required:

Implementing Libraries MUST support at minimum TTL functionality as described below with whole-second granularity.
[…]
Implementing Libraries MAY expire an item before its requested Expiration Time, but MUST treat an item as expired once its Expiration Time is reached.

vs.

If the underlying implementation does not support TTL, the user-specified TTL MUST be silently ignored.

Also, unlike getMultiple, the docstring for get does not say whether stale/expired items should be returned.

A little note from my side: HTTP compliance and compatibility with existing SimplePie code bases should be much more important than whatever PSR 16 might be saying

Well, if PSR 16 implementations all support TTL, we can remove the handling from everywhere except the legacy shim and it should still work.

@Art4 Why in bb38d74 did you start to set cache_expiration_time as part of the data in the first place? Is it to handle implementations that do not support TTL?

PSR 16 is inconsistent whether that is required:

Implementing Libraries MUST support at minimum TTL functionality as described below with whole-second granularity.
[…]
Implementing Libraries MAY expire an item before its requested Expiration Time, but MUST treat an item as expired once its Expiration Time is reached.

vs.

If the underlying implementation does not support TTL, the user-specified TTL MUST be silently ignored.

You are mixing up "Implementing Libraries" (Psr\SimpleCache\CacheInterface implementations) and "underlying implementation" (SimplePie\Cache\BaseDataCache).

cache_expiration_time was added to mimic the functionality or mtime() and touch() on the SimplePie side. In the SimplePie\Cache\BaseDataCache the TTL was implemented using an internal __cache_expiration_time key.

I am talking about the Implementing Libraries – if the Implementing Libraries MUST support TTL and the underlying backend does not support it, the implementing library has no other option than to emulate it (as we are doing with __cache_expiration_time). But it is not clear to me why it also says that “the user-specified TTL MUST be silently ignored”.

But the thing that was confusing me most is that we want the cache to return expired entries, which Psr16 cache does not support. I think I finally understand that after digging into #883 (comment)

We have already had a similar discussion https://github.com/FreshRSS/simplepie/pull/11/files#r1725726684
My impression is that there is a combination of bugs and more than one misunderstanding (very possibly one of them being from my side :-))
I would though like to see the straightforward #883 (or this one, but less ready) PR merged before embarking on tackling those issues

But the thing that was confusing me most is that we want the cache to return expired entries, which Psr16 cache does not support. I think I finally understand that after digging into #883 (comment)

Yes, that might be the biggest issue. Maybe atm the best would be to pass null (unlimited) as TTL, as you suggested here and only work with $data['cache_expiration_time'].

Art4 · 2024-09-27T20:24:14Z

Replaced by #883.

stop legacy file cache constantly updating

94d6a96

pull-request-size bot added the size/XS label Aug 28, 2023

Art4 mentioned this pull request Aug 28, 2023

[BUG] Cache constantly updating #830

Closed

Art4 added 2 commits August 28, 2023 10:05

fix cache_expiration_time in integration test

5c871cc

Revert "fix cache_expiration_time in integration test"

166cfac

This reverts commit 5c871cc.

Art4 marked this pull request as draft August 28, 2023 12:21

Art4 mentioned this pull request Sep 13, 2023

Release 1.9.0 #850

Merged

7 tasks

Art4 added 3 commits December 19, 2023 08:17

Merge branch 'master' into 830-fix-deprecated-file-cache-constantly-u…

05a6636

…pdating

Merge branch 'master' into 830-fix-deprecated-file-cache-constantly-u…

8813a8b

…pdating

Merge branch 'master' into 830-fix-deprecated-file-cache-constantly-u…

37c1d0d

…pdating

Art4 mentioned this pull request Aug 22, 2024

Hash-based caching FreshRSS/simplepie#11

Merged

Alkarex added a commit to FreshRSS/simplepie that referenced this pull request Sep 14, 2024

Merge SimplePie fix 846

bd3f797

simplepie#846

Alkarex mentioned this pull request Sep 14, 2024

Simplepie846 FreshRSS/simplepie#23

Merged

Alkarex reviewed Sep 14, 2024

View reviewed changes

Alkarex added a commit to FreshRSS/simplepie that referenced this pull request Sep 14, 2024

Simplepie846 (#23)

8458f8d

* Merge SimplePie fix 846 simplepie#846 * Fix tests running in the same second

Alkarex added a commit to FreshRSS/simplepie that referenced this pull request Sep 14, 2024

Fix cache_expiration_time

47d2cbc

fix simplepie#830 Replaces simplepie#846

Alkarex mentioned this pull request Sep 14, 2024

Fix cache_expiration_time #883

Merged

jtojnar reviewed Sep 25, 2024

View reviewed changes

Merge branch 'master' into 830-fix-deprecated-file-cache-constantly-u…

db5bd42

…pdating

jtojnar mentioned this pull request Sep 26, 2024

Release 1.8.1 #886

Merged

4 tasks

jtojnar reviewed Sep 26, 2024

View reviewed changes

Art4 closed this Sep 27, 2024

	elseif (isset($this->data['cache_expiration_time']) && $this->data['cache_expiration_time'] < time()) {
	elseif (empty($this->data['cache_expiration_time']) \|\| $this->data['cache_expiration_time'] < time()) {

	$defaultMtime = time();
	$defaultExpirationTime = $defaultMtime + 3600;

Conversation

Art4 commented Aug 28, 2023

Uh oh!

Art4 commented Aug 28, 2023

Uh oh!

Art4 commented Aug 28, 2023

Uh oh!

jtojnar commented Sep 4, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtojnar Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Art4 commented Sep 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jtojnar Sep 26, 2024 •

edited

Loading