Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Cache] give 100ms before starting the expiration countdown #33846

Merged
merged 1 commit into from Oct 4, 2019

Conversation

@nicolas-grekas
Copy link
Member

commented Oct 4, 2019

Q A
Branch? 4.3
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #31573, Fix #33837
License MIT
Doc PR -

Because the expiration count-down starts immediately after calling CachItem::expiresAfter(N), it's impossible to actually cache items for more than N-1 seconds.

This PR adds a 0.1s grace period so that backends that have a second-level resolution can store the items for N seconds, provided the time between calling CachItem::expiresAfter(N) and saving the value to the backend is lower than 0.1s.

This PR also fixes the calculation of the computation time in ContractsTrait.

@@ -76,15 +76,15 @@ static function ($deferred, $namespace, &$expiredIds) use ($getId) {
$key = (string) $key;
if (null === $item->expiry) {
$ttl = 0 < $item->defaultLifetime ? $item->defaultLifetime : 0;
} elseif (0 >= $ttl = (int) ($item->expiry - $now)) {
} elseif (0 >= $ttl = (int) (0.1 + $item->expiry - $now)) {

This comment has been minimized.

Copy link
@javiereguiluz

javiereguiluz Oct 4, 2019

Member

This could look like a "magic number" in the future and people won't understand why we use it. We could add a very brief comment explaining "why" ... or we could move this to a self-explanatory private constant (e.g. self::MINIMUM_CACHE_TTL or something better).

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Oct 4, 2019

Author Member

The value is spread in many unrelated adapters, a private constant doesn't work.
I'm not sure this deserves more honestly, git blame can tell why if anyone wonders.

nicolas-grekas added a commit that referenced this pull request Oct 4, 2019
…n (nicolas-grekas)

This PR was merged into the 4.3 branch.

Discussion
----------

[Cache] give 100ms before starting the expiration countdown

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #31573, Fix #33837
| License       | MIT
| Doc PR        | -

Because the expiration count-down starts immediately after calling `CachItem::expiresAfter(N)`, it's impossible to actually cache items for more than `N-1` seconds.

This PR adds a 0.1s grace period so that backends that have a second-level resolution can store the items for `N` seconds, provided the time between calling `CachItem::expiresAfter(N)` and saving the value to the backend is lower than 0.1s.

This PR also fixes the calculation of the computation time in `ContractsTrait`.

Commits
-------

ba63e18 [Cache] give 100ms before starting the expiration countdown
@nicolas-grekas nicolas-grekas merged commit ba63e18 into symfony:4.3 Oct 4, 2019
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details
@fabpot fabpot referenced this pull request Oct 7, 2019
@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:cache-expiry branch Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.