Modules: fix SharedDict.pop() for unexpired keys in timeout zones

Previously, SharedDict.pop() removed an entry from the expiration rbtree
before checking whether it had expired. ngx_rbtree_delete() clears the
removed node's key, so the subsequent expiration check treated every
entry in a timeout zone as expired. pop() therefore deleted unexpired
entries but returned undefined instead of their values.

The fix preserves the expiration key before removing the node and uses
the saved value for the check in both the njs and QuickJS
implementations. Tests are added for popping an unexpired entry from a
timeout zone.

diff --git a/nginx/ngx_js_shared_dict.c b/nginx/ngx_js_shared_dict.c
index d9a1fc7c4ca1201e58c7329992738365ccda0113..b4b16b98d6fe8511c6275ac1f37f7701ac04b8ca 100644 (file)
--- a/nginx/ngx_js_shared_dict.c
+++ b/nginx/ngx_js_shared_dict.c
@@ -1429,6 +1429,20 @@ ngx_js_dict_node_free(ngx_js_dict_t *dict, ngx_js_dict_node_t *node)
 }
 
 
+static ngx_rbtree_key_t
+ngx_js_dict_expire_unlink(ngx_js_dict_t *dict, ngx_js_dict_node_t *node)
+{
+    ngx_rbtree_key_t  expire;
+
+    /* ngx_rbtree_delete() clears the node key. */
+    expire = node->expire.key;
+
+    ngx_rbtree_delete(&dict->sh->rbtree_expire, &node->expire);
+
+    return expire;
+}
+
+
 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)
@@ -1618,6 +1632,7 @@ 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;
 
     ngx_rwlock_wlock(&dict->sh->rwlock);
@@ -1629,8 +1644,10 @@ ngx_js_dict_delete(njs_vm_t *vm, ngx_js_dict_t *dict, ngx_str_t *key,
         return NGX_DECLINED;
     }
 
+    expire = 0;
+
     if (dict->timeout) {
-        ngx_rbtree_delete(&dict->sh->rbtree_expire, &node->expire);
+        expire = ngx_js_dict_expire_unlink(dict, node);
     }
 
     ngx_rbtree_delete(&dict->sh->rbtree, (ngx_rbtree_node_t *) node);
@@ -1639,7 +1656,7 @@ ngx_js_dict_delete(njs_vm_t *vm, ngx_js_dict_t *dict, ngx_str_t *key,
         tp = ngx_timeofday();
         now = tp->sec * 1000 + tp->msec;
 
-        if (!dict->timeout || now < node->expire.key) {
+        if (!dict->timeout || now < expire) {
             rc = ngx_js_dict_copy_value_locked(vm, dict, node, retval);
 
         } else {
@@ -4041,6 +4058,7 @@ 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;
 
     ngx_rwlock_wlock(&dict->sh->rwlock);
@@ -4052,8 +4070,10 @@ ngx_qjs_dict_delete(JSContext *cx, ngx_js_dict_t *dict, ngx_str_t *key,
         return JS_UNDEFINED;
     }
 
+    expire = 0;
+
     if (dict->timeout) {
-        ngx_rbtree_delete(&dict->sh->rbtree_expire, &node->expire);
+        expire = ngx_js_dict_expire_unlink(dict, node);
     }
 
     ngx_rbtree_delete(&dict->sh->rbtree, (ngx_rbtree_node_t *) node);
@@ -4062,7 +4082,7 @@ ngx_qjs_dict_delete(JSContext *cx, ngx_js_dict_t *dict, ngx_str_t *key,
         tp = ngx_timeofday();
         now = tp->sec * 1000 + tp->msec;
 
-        if (!dict->timeout || now < node->expire.key) {
+        if (!dict->timeout || now < expire) {
             ret = ngx_qjs_dict_copy_value_locked(cx, dict, node);
 
         } else {

diff --git a/nginx/t/js_shared_dict.t b/nginx/t/js_shared_dict.t
index b30e2865b9be08ba00cb00a43668b911dafc398a..0fb05db4e21c9081257263c63964bb93ec8122f9 100644 (file)
--- a/nginx/t/js_shared_dict.t
+++ b/nginx/t/js_shared_dict.t
@@ -356,7 +356,7 @@ EOF
 
 $t->try_run('no js_shared_dict_zone');
 
-$t->plan(59);
+$t->plan(61);
 
 ###############################################################################
 
@@ -482,6 +482,11 @@ ok($ttl_incr_def >= 900000 && $ttl_incr_def <= 1000000,
 
 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');
+
+http_get('/set?dict=foo&key=POP&value=alive');
+like(http_get('/pop?dict=foo&key=POP'), qr/alive/, 'pop unexpired foo.POP');
+like(http_get('/pop?dict=foo&key=POP'), qr/undefined/, 'pop deleted foo.POP');
+
 http_get('/set?dict=foo&key=BAR&value=xxx');
 like(http_get('/clear?dict=foo'), qr/undefined/, 'clear foo');