Skip to content

Feature/apcu_set_ttl Add function to set the TTL of an existing entry atomically #507

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Feature/apcu_set_ttl Add function to set the TTL of an existing entry atomically #507

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c72e690
Implement apcu_set_ttl
Mar 29, 2024
7a28de6
Implement apcu_set_ttl
ignas2526 Mar 29, 2024
001f7b6
Implement apcu_set_ttl
ignas2526 Mar 29, 2024
42e63a7
Fix definition
ignas2526 Mar 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 29 additions & 1 deletion apc_cache.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ PHP_APCU_API int APC_UNSERIALIZER_NAME(php) (APC_UNSERIALIZER_ARGS)
result = php_var_unserialize(value, &tmp, buf + buf_len, &var_hash);
PHP_VAR_UNSERIALIZE_DESTROY(var_hash);
BG(serialize_lock)--;

if (!result) {
php_error_docref(NULL, E_NOTICE, "Error at offset %td of %zd bytes", tmp - buf, buf_len);
ZVAL_NULL(value);
Expand Down Expand Up @@ -539,6 +539,34 @@ PHP_APCU_API zend_bool apc_cache_store(
return ret;
} /* }}} */

/* {{{ apc_cache_set_ttl */
PHP_APCU_API zend_bool apc_cache_update_ttl(
apc_cache_t* cache, zend_string *key, const int32_t ttl) {
Copy link
Collaborator

@nikic nikic Jun 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
apc_cache_t* cache, zend_string *key, const int32_t ttl) {
apc_cache_t *cache, zend_string *key, int32_t ttl) {

apc_cache_entry_t *entry;
zend_bool retval = 0;
time_t t = apc_time();

if (!cache) {
return 0;
}

if (!apc_cache_rlock(cache)) {
return 0;
}

entry = apc_cache_rlocked_find_nostat(cache, key, t);
if (!entry) {
apc_cache_runlock(cache);
return 0;
}

entry->ctime = t;
Copy link
Collaborator

@nikic nikic Jun 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, should this really be setting ctime and not mtime (or neither)?

Copy link
Contributor

@madmajestro madmajestro Apr 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Author

@ignas2526 ignas2526 Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@madmajestro madmajestro Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Collaborator

@nikic nikic Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using mtime sounds reasonable to me.

entry->ttl = ttl;

apc_cache_runlock(cache);
return 1;
} /* }}} */

#ifndef ZTS
/* {{{ data_unserialize */
static zval data_unserialize(const char *filename)
Expand Down
7 changes: 7 additions & 0 deletions apc_cache.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,13 @@ PHP_APCU_API void apc_cache_clear(apc_cache_t* cache);
PHP_APCU_API zend_bool apc_cache_store(
apc_cache_t* cache, zend_string *key, const zval *val,
const int32_t ttl, const zend_bool exclusive);

/*
* apc_cache_update_ttl updates ttl of the key, if it exists
*/
PHP_APCU_API zend_bool apc_cache_update_ttl(
apc_cache_t* cache, zend_string *key, const int32_t ttl);

/*
* apc_cache_update updates an entry in place. The updater function must not bailout.
* The update is performed under write-lock and doesn't have to be atomic.
Expand Down
20 changes: 20 additions & 0 deletions php_apc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -517,6 +517,26 @@ PHP_FUNCTION(apcu_store) {
}
/* }}} */

/* {{{ proto bool apc_cache_update_ttl(mixed key [, long ttl ])
*/
PHP_FUNCTION(apcu_set_ttl) {
zend_string *key;
zend_long ttl = 0L;
zval *success = NULL;
Copy link
Collaborator

@nikic nikic Jun 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused?

time_t t;

ZEND_PARSE_PARAMETERS_START(1, 2)
Z_PARAM_STR(key)
Z_PARAM_OPTIONAL
Z_PARAM_LONG(ttl)
Copy link
Collaborator

@nikic nikic Jun 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is ttl optional?

ZEND_PARSE_PARAMETERS_END();

t = apc_time();
Copy link
Collaborator

@nikic nikic Jun 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused?


RETURN_BOOL(apc_cache_update_ttl(apc_user_cache, key, ttl));
}
/* }}} */

/* {{{ proto int apcu_add(mixed key, mixed var [, long ttl ])
*/
PHP_FUNCTION(apcu_add) {
Expand Down
2 changes: 2 additions & 0 deletions php_apc.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ function apcu_enabled(): bool {}
/** @param array|string $key */
function apcu_store($key, mixed $value = UNKNOWN, int $ttl = 0): array|bool {}

function apcu_set_ttl(string $key, int $ttl = 0): bool {}

/** @param array|string $key */
function apcu_add($key, mixed $value = UNKNOWN, int $ttl = 0): array|bool {}

Expand Down
7 changes: 7 additions & 0 deletions php_apc_arginfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ ZEND_END_ARG_INFO()

#define arginfo_apcu_add arginfo_apcu_store

ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_apcu_set_ttl, 0, 1, _IS_BOOL, 0)
ZEND_ARG_INFO(0, key)
ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, ttl, IS_LONG, 0, "0")
ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_WITH_RETURN_TYPE_MASK_EX(arginfo_apcu_inc, 0, 1, MAY_BE_LONG|MAY_BE_FALSE)
ZEND_ARG_TYPE_INFO(0, key, IS_STRING, 0)
ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, step, IS_LONG, 0, "1")
Expand Down Expand Up @@ -69,6 +74,7 @@ PHP_APCU_API ZEND_FUNCTION(apcu_key_info);
PHP_APCU_API ZEND_FUNCTION(apcu_sma_info);
PHP_APCU_API ZEND_FUNCTION(apcu_enabled);
PHP_APCU_API ZEND_FUNCTION(apcu_store);
PHP_APCU_API ZEND_FUNCTION(apcu_set_ttl);
PHP_APCU_API ZEND_FUNCTION(apcu_add);
PHP_APCU_API ZEND_FUNCTION(apcu_inc);
PHP_APCU_API ZEND_FUNCTION(apcu_dec);
Expand All @@ -89,6 +95,7 @@ static const zend_function_entry ext_functions[] = {
ZEND_FE(apcu_sma_info, arginfo_apcu_sma_info)
ZEND_FE(apcu_enabled, arginfo_apcu_enabled)
ZEND_FE(apcu_store, arginfo_apcu_store)
ZEND_FE(apcu_set_ttl, arginfo_apcu_set_ttl)
ZEND_FE(apcu_add, arginfo_apcu_add)
ZEND_FE(apcu_inc, arginfo_apcu_inc)
ZEND_FE(apcu_dec, arginfo_apcu_dec)
Expand Down