Feature/apcu_set_ttl Add function to set the TTL of an existing entry atomically#507
Feature/apcu_set_ttl Add function to set the TTL of an existing entry atomically#507ignas2526 wants to merge 4 commits intokrakjoe:masterfrom
Conversation
nikic
left a comment
nikic
left a comment
There was a problem hiding this comment.
The feature sounds reasonable to me. This is currently missing a test though.
|
|
||
| /* {{{ apc_cache_set_ttl */ | ||
| PHP_APCU_API zend_bool apc_cache_update_ttl( | ||
| apc_cache_t* cache, zend_string *key, const int32_t ttl) { |
There was a problem hiding this comment.
| apc_cache_t* cache, zend_string *key, const int32_t ttl) { | |
| apc_cache_t *cache, zend_string *key, int32_t ttl) { |
| Z_PARAM_LONG(ttl) | ||
| ZEND_PARSE_PARAMETERS_END(); | ||
|
|
||
| t = apc_time(); |
| PHP_FUNCTION(apcu_set_ttl) { | ||
| zend_string *key; | ||
| zend_long ttl = 0L; | ||
| zval *success = NULL; |
| ZEND_PARSE_PARAMETERS_START(1, 2) | ||
| Z_PARAM_STR(key) | ||
| Z_PARAM_OPTIONAL | ||
| Z_PARAM_LONG(ttl) |
| return 0; | ||
| } | ||
|
|
||
| entry->ctime = t; |
There was a problem hiding this comment.
Hm, should this really be setting ctime and not mtime (or neither)?
There was a problem hiding this comment.
@nikic
I'm wondering if it wouldn't be generally better to change the behavior of apc_cache_entry_hard_expired() and apc_iterator_check_expiry() to check entries based on mtime instead of ctime.
In my opinion, it makes sense to calculate the TTL from the time of the last modification. Otherwise, you change something just before expiration, and then the entry is gone. That doesn't make sense to me.
There was a problem hiding this comment.
So, my idea was to have a way to extend TTL of an entry. The only way to do it is to either change ctime or mtime as far as I understand. I don't think we should add new timestamp to track lifespan and make things complicated.
If I remember correctly, my idea was to do what apcu_add/apcu_store would do if it was called with the same values.
There was a problem hiding this comment.
It is not my plan to introduce a new timestamp. But ctime should imho not be changed because it is the creation timestamp and not the modification timestamp. So my plan is to refresh mtime. But this currently makes setting ttl work not as expected as you would have to increase ttl with each ttl-update if you just plan to restart timer withe same ttl. The Solution is to change all expiery checks to mtime, which imho generally makes sense...
There was a problem hiding this comment.
Using mtime sounds reasonable to me.
|
@ignas2526 |
|
Yea sure, you can take over if you have time. I think this PR is mostly done and the idea is outlined already. So you should have everything to wrap it up. |
|
@ignas2526 |
3 participants
APCu lacks a nice way to update the TTL of an entry. Currently, you'd have to implement your own critical section, fetch the value of an entry, and then store it back. I think there are many use cases with websites where one would want to create a cache entry with a short life and then extend its lifespan on a subsequent request from the browser.
I think this will be especially useful in combination with apcu_inc, apcu_dec, and apcu_cas. Because calling apcu_store (the only way to extend TTL ATM) after them defeats their purpose.
This PR adds
apcu_set_ttl($key, int $ttl = 0): bool. If entry with the $key has been successfully updated, function returns true, else, false.