Modules: treat expired shared dict entries as absent

Expired entries are hidden by read APIs but can still be found by
write operations.  replace() can revive an expired entry, delete()
can report that it existed, and incr() can use its old value instead
of init.  Without an explicit timeout, incr() can return a value that
a subsequent get() cannot retrieve.

Use a common write-side lookup to remove expired entries and handle
them as missing in both engines.  incr() then creates a new entry
with init plus delta and the directive or explicit timeout.

Also return false instead of undefined when QuickJS delete() is
called for a missing key.

diff --git a/nginx/ngx_js_shared_dict.c b/nginx/ngx_js_shared_dict.c
index 5c14d67c2b8ac8ac7515eb0eec9dd42fb39c282c..355f4e51ce0336d68ee3aed8f5a1ec59d9bc9bab 100644 (file)
--- a/nginx/ngx_js_shared_dict.c
+++ b/nginx/ngx_js_shared_dict.c
@@ -1455,6 +1455,24 @@ ngx_js_dict_expire_link(ngx_js_dict_t *dict, ngx_js_dict_node_t *node,
 }
 
 
+static ngx_js_dict_node_t *
+ngx_js_dict_lookup_write(ngx_js_dict_t *dict, ngx_str_t *key, ngx_msec_t now)
+{
+    ngx_js_dict_node_t  *node;
+
+    node = ngx_js_dict_lookup(dict, key);
+
+    if (node != NULL && dict->timeout && now >= node->expire.key) {
+        ngx_js_dict_expire_unlink(dict, node);
+        ngx_rbtree_delete(&dict->sh->rbtree, &node->sn.node);
+        ngx_js_dict_node_free(dict, node);
+        return NULL;
+    }
+
+    return node;
+}
+
+
 static ngx_int_t
 ngx_js_dict_set(njs_vm_t *vm, ngx_js_dict_t *dict, ngx_str_t *key,
     njs_value_t *value, ngx_msec_t timeout, unsigned flags)
@@ -1468,7 +1486,7 @@ ngx_js_dict_set(njs_vm_t *vm, ngx_js_dict_t *dict, ngx_str_t *key,
 
     ngx_rwlock_wlock(&dict->sh->rwlock);
 
-    node = ngx_js_dict_lookup(dict, key);
+    node = ngx_js_dict_lookup_write(dict, key, now);
 
     if (node == NULL) {
         if (flags & NGX_JS_DICT_FLAG_MUST_EXIST) {
@@ -1482,10 +1500,8 @@ ngx_js_dict_set(njs_vm_t *vm, ngx_js_dict_t *dict, ngx_str_t *key,
 
     } else {
         if (flags & NGX_JS_DICT_FLAG_MUST_NOT_EXIST) {
-            if (!dict->timeout || now < node->expire.key) {
-                ngx_rwlock_unlock(&dict->sh->rwlock);
-                return NGX_DECLINED;
-            }
+            ngx_rwlock_unlock(&dict->sh->rwlock);
+            return NGX_DECLINED;
         }
 
         if (ngx_js_dict_update(vm, dict, node, value, timeout, now) != NGX_OK) {
@@ -1668,36 +1684,28 @@ ngx_js_dict_delete(njs_vm_t *vm, ngx_js_dict_t *dict, ngx_str_t *key,
     ngx_int_t            rc;
     ngx_msec_t           now;
     ngx_time_t          *tp;
-    ngx_rbtree_key_t     expire;
     ngx_js_dict_node_t  *node;
 
+    tp = ngx_timeofday();
+    now = tp->sec * 1000 + tp->msec;
+
     ngx_rwlock_wlock(&dict->sh->rwlock);
 
-    node = ngx_js_dict_lookup(dict, key);
+    node = ngx_js_dict_lookup_write(dict, key, now);
 
     if (node == NULL) {
         ngx_rwlock_unlock(&dict->sh->rwlock);
         return NGX_DECLINED;
     }
 
-    expire = 0;
-
     if (dict->timeout) {
-        expire = ngx_js_dict_expire_unlink(dict, node);
+        ngx_js_dict_expire_unlink(dict, node);
     }
 
     ngx_rbtree_delete(&dict->sh->rbtree, (ngx_rbtree_node_t *) node);
 
     if (retval != NULL) {
-        tp = ngx_timeofday();
-        now = tp->sec * 1000 + tp->msec;
-
-        if (!dict->timeout || now < expire) {
-            rc = ngx_js_dict_copy_value_locked(vm, dict, node, retval);
-
-        } else {
-            rc = NGX_DECLINED;
-        }
+        rc = ngx_js_dict_copy_value_locked(vm, dict, node, retval);
 
     } else {
         rc = NGX_OK;
@@ -1730,7 +1738,7 @@ ngx_js_dict_incr(njs_vm_t *vm, ngx_js_dict_t *dict, ngx_str_t *key,
 
     ngx_rwlock_wlock(&dict->sh->rwlock);
 
-    node = ngx_js_dict_lookup(dict, key);
+    node = ngx_js_dict_lookup_write(dict, key, now);
 
     if (node == NULL) {
         njs_value_number_set(init, njs_value_number(init)
@@ -4141,36 +4149,28 @@ ngx_qjs_dict_delete(JSContext *cx, ngx_js_dict_t *dict, ngx_str_t *key,
     JSValue              ret;
     ngx_msec_t           now;
     ngx_time_t          *tp;
-    ngx_rbtree_key_t     expire;
     ngx_js_dict_node_t  *node;
 
+    tp = ngx_timeofday();
+    now = tp->sec * 1000 + tp->msec;
+
     ngx_rwlock_wlock(&dict->sh->rwlock);
 
-    node = ngx_qjs_dict_lookup(dict, key);
+    node = ngx_js_dict_lookup_write(dict, key, now);
 
     if (node == NULL) {
         ngx_rwlock_unlock(&dict->sh->rwlock);
-        return JS_UNDEFINED;
+        return retval ? JS_UNDEFINED : JS_FALSE;
     }
 
-    expire = 0;
-
     if (dict->timeout) {
-        expire = ngx_js_dict_expire_unlink(dict, node);
+        ngx_js_dict_expire_unlink(dict, node);
     }
 
     ngx_rbtree_delete(&dict->sh->rbtree, (ngx_rbtree_node_t *) node);
 
     if (retval) {
-        tp = ngx_timeofday();
-        now = tp->sec * 1000 + tp->msec;
-
-        if (!dict->timeout || now < expire) {
-            ret = ngx_qjs_dict_copy_value_locked(cx, dict, node);
-
-        } else {
-            ret = JS_UNDEFINED;
-        }
+        ret = ngx_qjs_dict_copy_value_locked(cx, dict, node);
 
     } else {
         ret = JS_TRUE;
@@ -4242,7 +4242,7 @@ ngx_qjs_dict_incr(JSContext *cx, ngx_js_dict_t *dict, ngx_str_t *key,
 
     ngx_rwlock_wlock(&dict->sh->rwlock);
 
-    node = ngx_qjs_dict_lookup(dict, key);
+    node = ngx_js_dict_lookup_write(dict, key, now);
 
     if (node == NULL) {
         value = JS_NewFloat64(cx, init + delta);
@@ -4291,7 +4291,7 @@ ngx_qjs_dict_set(JSContext *cx, ngx_js_dict_t *dict, ngx_str_t *key,
 
     ngx_rwlock_wlock(&dict->sh->rwlock);
 
-    node = ngx_qjs_dict_lookup(dict, key);
+    node = ngx_js_dict_lookup_write(dict, key, now);
 
     if (node == NULL) {
         if (flags & NGX_JS_DICT_FLAG_MUST_EXIST) {
@@ -4306,10 +4306,8 @@ ngx_qjs_dict_set(JSContext *cx, ngx_js_dict_t *dict, ngx_str_t *key,
     } else {
 
         if (flags & NGX_JS_DICT_FLAG_MUST_NOT_EXIST) {
-            if (!dict->timeout || now < node->expire.key) {
-                ngx_rwlock_unlock(&dict->sh->rwlock);
-                return JS_FALSE;
-            }
+            ngx_rwlock_unlock(&dict->sh->rwlock);
+            return JS_FALSE;
         }
 
         if (ngx_qjs_dict_update(cx, dict, node, value, timeout, now)

diff --git a/nginx/t/js_shared_dict.t b/nginx/t/js_shared_dict.t
index 414becf538543096bc53de8f462cdbc41be9433c..f759a8d5a4998c8b2286cd096972984ed28a8633 100644 (file)
--- a/nginx/t/js_shared_dict.t
+++ b/nginx/t/js_shared_dict.t
@@ -384,7 +384,7 @@ EOF
 
 $t->try_run('no js_shared_dict_zone');
 
-$t->plan(64);
+$t->plan(77);
 
 ###############################################################################
 
@@ -432,10 +432,18 @@ like(http_get('/replace?dict=foo&key=FOO2&value=aaa'), qr/true/,
 like(http_get('/replace?dict=foo&key=NOT_EXISTING&value=aaa'), qr/false/,
        'failed replace foo.NOT_EXISTING');
 
+http_get('/set?dict=foo&key=DELETE&value=xxx');
+http_get('/set?dict=foo&key=POP&value=xxx');
+
 select undef, undef, undef, 2.1;
 
-like(http_get('/get?dict=foo&key=FOO'), qr/undefined/, 'get expired foo.FOO');
-like(http_get('/pop?dict=foo&key=FOO'), qr/undefined/, 'pop expired foo.FOO');
+like(http_get('/get?dict=foo&key=FOO2'), qr/undefined/,
+       'get expired foo.FOO2');
+like(http_get('/pop?dict=foo&key=POP'), qr/undefined/, 'pop expired foo.POP');
+like(http_get('/has?dict=foo&key=FOO3'), qr/false/,
+       'has expired foo.FOO3');
+like(http_get('/ttl?dict=foo&key=FOO3'), qr/undefined/,
+       'ttl expired foo.FOO3');
 
 TODO: {
 local $TODO = 'not yet' unless has_version('0.8.1');
@@ -444,6 +452,8 @@ like(http_get('/keys?dict=foo'), qr/empty/, 'foo keys after expire');
 like(http_get('/keys?dict=bar'), qr/FOO\,FOO2/, 'bar keys after a delay');
 like(http_get('/size?dict=foo'), qr/size: 0/,
        'no of items in foo after expire');
+like(http_get('/items?dict=foo'), qr/\x0d\x0a\x0d\x0a$/,
+       'foo items after expire');
 like(http_get('/items?dict=bar'), qr/FOO,zzz|FOO2,aaa/,
        'bar items');
 like(http_get('/items?dict=waka'), qr/FOO,47|FOO2,7779|FOO3,3338/,
@@ -451,6 +461,20 @@ like(http_get('/items?dict=waka'), qr/FOO,47|FOO2,7779|FOO3,3338/,
 
 }
 
+TODO: {
+local $TODO = 'not yet' unless has_version('1.0.3');
+
+like(http_get('/replace?dict=foo&key=FOO2&value=aaa'), qr/false/,
+       'replace expired foo.FOO2');
+like(http_get('/get?dict=foo&key=FOO2'), qr/undefined/,
+       'failed replace does not revive foo.FOO2');
+like(http_get('/delete?dict=foo&key=DELETE'), qr/false/,
+       'delete expired foo.DELETE');
+like(http_get('/delete?dict=foo&key=DELETE'), qr/false/,
+       'delete missing foo.DELETE');
+
+}
+
 TODO: {
 local $TODO = 'not yet' unless has_version('0.8.5');
 
@@ -508,6 +532,38 @@ my $ttl_incr_def = get_ttl('/ttl?dict=waka&key=TTL_INCR_DEF');
 ok($ttl_incr_def >= 900000 && $ttl_incr_def <= 1000000,
        'incr without timeout uses directive default');
 
+http_get('/set?dict=waka&key=EXPIRED&value=100&timeout=100');
+select undef, undef, undef, 0.2;
+
+TODO: {
+local $TODO = 'not yet' unless has_version('1.0.3');
+
+like(http_get('/incr?dict=waka&key=EXPIRED&by=3&def=7'), qr/^10$/m,
+       'incr expired entry uses init');
+like(http_get('/get?dict=waka&key=EXPIRED'), qr/^10$/m,
+       'incr expired entry creates visible value');
+my $ttl_expired = get_ttl('/ttl?dict=waka&key=EXPIRED');
+ok($ttl_expired >= 900000 && $ttl_expired <= 1000000,
+       'incr expired entry uses directive timeout');
+
+}
+
+http_get('/set?dict=waka&key=EXPIRED_EXPLICIT&value=100&timeout=100');
+select undef, undef, undef, 0.2;
+
+TODO: {
+local $TODO = 'not yet' unless has_version('1.0.3');
+
+like(http_get('/incr?dict=waka&key=EXPIRED_EXPLICIT&by=3&def=7&timeout=30000'),
+       qr/^10$/m, 'incr expired entry with explicit timeout uses init');
+like(http_get('/get?dict=waka&key=EXPIRED_EXPLICIT'), qr/^10$/m,
+       'incr expired entry with explicit timeout creates visible value');
+my $ttl_explicit = get_ttl('/ttl?dict=waka&key=EXPIRED_EXPLICIT');
+ok($ttl_explicit >= 25000 && $ttl_explicit <= 30000,
+       'incr expired entry uses explicit timeout');
+
+}
+
 like(http_get('/pop?dict=bar&key=FOO'), qr/zzz/, 'pop bar.FOO');
 like(http_get('/pop?dict=bar&key=FOO'), qr/undefined/, 'pop deleted bar.FOO');