Modules: preserve shared dict entries on failed updates

Shared dictionary updates temporarily remove timeout entries from the
expiration rbtree to prevent eviction while allocating a replacement.
ngx_rbtree_delete() clears the removed node key, so the failure path
reinserted the entry with a zero expiration and made its old value appear
expired.

Factor the shared-memory update into an engine-independent helper and
restore the previous expiration when allocation fails.  Keep value
conversion in the njs and QuickJS wrappers.

diff --git a/nginx/ngx_js_shared_dict.c b/nginx/ngx_js_shared_dict.c
index b4b16b98d6fe8511c6275ac1f37f7701ac04b8ca..49bf4a57c784e7d755525259dbc41bd7e537dfdc 100644 (file)
--- a/nginx/ngx_js_shared_dict.c
+++ b/nginx/ngx_js_shared_dict.c
@@ -1443,6 +1443,16 @@ ngx_js_dict_expire_unlink(ngx_js_dict_t *dict, ngx_js_dict_node_t *node)
 }
 
 
+static void
+ngx_js_dict_expire_link(ngx_js_dict_t *dict, ngx_js_dict_node_t *node,
+    ngx_rbtree_key_t expire)
+{
+    node->expire.key = expire;
+
+    ngx_rbtree_insert(&dict->sh->rbtree_expire, &node->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)
@@ -1577,51 +1587,75 @@ ngx_js_dict_add(njs_vm_t *vm, ngx_js_dict_t *dict, ngx_str_t *key,
 
 
 static ngx_int_t
-ngx_js_dict_update(njs_vm_t *vm, ngx_js_dict_t *dict, ngx_js_dict_node_t *node,
-    njs_value_t *value, ngx_msec_t timeout, ngx_msec_t now)
+ngx_js_dict_update_value(ngx_js_dict_t *dict, ngx_js_dict_node_t *node,
+    ngx_js_dict_value_t *value, ngx_msec_t timeout, ngx_msec_t now)
 {
-    u_char     *p;
-    njs_str_t   string;
+    u_char            *p;
+    ngx_rbtree_key_t   expire;
+
+    expire = 0;
 
     if (dict->timeout) {
         /*
-         * Remove the node from the expire tree before allocating
-         * memory for the new value.  This serves two purposes:
-         * it prevents ngx_js_dict_evict() from freeing this node
-         * during allocation, and it allows the node to be reinserted
-         * with the updated expiry time below.
+         * Keep the node out of the expire tree while allocation may
+         * evict entries.  This prevents self-eviction and allows the
+         * old expiration to be restored if allocation fails.
          */
-        ngx_rbtree_delete(&dict->sh->rbtree_expire, &node->expire);
+        expire = ngx_js_dict_expire_unlink(dict, node);
     }
 
     if (dict->type == NGX_JS_DICT_TYPE_STRING) {
-        njs_value_string_get(vm, value, &string);
-
-        p = ngx_js_dict_alloc(dict, string.length);
+        p = ngx_js_dict_alloc(dict, value->str.len);
         if (p == NULL) {
-            if (dict->timeout) {
-                ngx_rbtree_insert(&dict->sh->rbtree_expire, &node->expire);
-            }
-
-            return NGX_ERROR;
+            goto failed;
         }
 
         ngx_slab_free_locked(dict->shpool, node->value.str.data);
-        ngx_memcpy(p, string.start, string.length);
+        ngx_memcpy(p, value->str.data, value->str.len);
 
         node->value.str.data = p;
-        node->value.str.len = string.length;
+        node->value.str.len = value->str.len;
 
     } else {
-        node->value.number = njs_value_number(value);
+        node->value.number = value->number;
     }
 
     if (dict->timeout) {
-        node->expire.key = now + timeout;
-        ngx_rbtree_insert(&dict->sh->rbtree_expire, &node->expire);
+        ngx_js_dict_expire_link(dict, node, now + timeout);
     }
 
     return NGX_OK;
+
+failed:
+
+    if (dict->timeout) {
+        ngx_js_dict_expire_link(dict, node, expire);
+    }
+
+    return NGX_ERROR;
+}
+
+
+static ngx_int_t
+ngx_js_dict_update(njs_vm_t *vm, ngx_js_dict_t *dict, ngx_js_dict_node_t *node,
+    njs_value_t *value, ngx_msec_t timeout, ngx_msec_t now)
+{
+    njs_str_t            string;
+    ngx_js_dict_value_t  entry;
+
+    if (dict->type == NGX_JS_DICT_TYPE_STRING) {
+        njs_value_string_get(vm, value, &string);
+
+        entry.str.data = string.start;
+        entry.str.len = string.length;
+
+    } else {
+        /* GCC complains about uninitialized entry.str.data. */
+        entry.str.data = NULL;
+        entry.number = njs_value_number(value);
+    }
+
+    return ngx_js_dict_update_value(dict, node, &entry, timeout, now);
 }
 
 
@@ -4258,65 +4292,31 @@ static ngx_int_t
 ngx_qjs_dict_update(JSContext *cx, ngx_js_dict_t *dict,
     ngx_js_dict_node_t *node, JSValue value, ngx_msec_t timeout, ngx_msec_t now)
 {
-    u_char     *p;
-    ngx_str_t   string;
-
-    if (dict->timeout) {
-        /*
-         * Remove the node from the expire tree before allocating
-         * memory for the new value.  This serves two purposes:
-         * it prevents ngx_js_dict_evict() from freeing this node
-         * during allocation, and it allows the node to be reinserted
-         * with the updated expiry time below.
-         */
-        ngx_rbtree_delete(&dict->sh->rbtree_expire, &node->expire);
-    }
+    ngx_int_t            rc;
+    ngx_js_dict_value_t  entry;
 
     if (dict->type == NGX_JS_DICT_TYPE_STRING) {
-        string.data = (u_char *) JS_ToCStringLen(cx, &string.len, value);
-        if (string.data == NULL) {
-            if (dict->timeout) {
-                ngx_rbtree_insert(&dict->sh->rbtree_expire, &node->expire);
-            }
-
-            return NGX_ERROR;
-        }
-
-        p = ngx_js_dict_alloc(dict, string.len);
-        if (p == NULL) {
-            JS_FreeCString(cx, (char *) string.data);
-
-            if (dict->timeout) {
-                ngx_rbtree_insert(&dict->sh->rbtree_expire, &node->expire);
-            }
-
+        entry.str.data = (u_char *) JS_ToCStringLen(cx, &entry.str.len, value);
+        if (entry.str.data == NULL) {
             return NGX_ERROR;
         }
 
-        ngx_slab_free_locked(dict->shpool, node->value.str.data);
-        ngx_memcpy(p, string.data, string.len);
-
-        node->value.str.data = p;
-        node->value.str.len = string.len;
-
-        JS_FreeCString(cx, (char *) string.data);
-
     } else {
-        if (JS_ToFloat64(cx, &node->value.number, value) < 0) {
-            if (dict->timeout) {
-                ngx_rbtree_insert(&dict->sh->rbtree_expire, &node->expire);
-            }
+        /* GCC complains about uninitialized entry.str.data. */
+        entry.str.data = NULL;
 
+        if (JS_ToFloat64(cx, &entry.number, value) < 0) {
             return NGX_ERROR;
         }
     }
 
-    if (dict->timeout) {
-        node->expire.key = now + timeout;
-        ngx_rbtree_insert(&dict->sh->rbtree_expire, &node->expire);
+    rc = ngx_js_dict_update_value(dict, node, &entry, timeout, now);
+
+    if (dict->type == NGX_JS_DICT_TYPE_STRING) {
+        JS_FreeCString(cx, (char *) entry.str.data);
     }
 
-    return NGX_OK;
+    return rc;
 }
 
 

diff --git a/nginx/t/js_shared_dict.t b/nginx/t/js_shared_dict.t
index 0fb05db4e21c9081257263c63964bb93ec8122f9..414becf538543096bc53de8f462cdbc41be9433c 100644 (file)
--- a/nginx/t/js_shared_dict.t
+++ b/nginx/t/js_shared_dict.t
@@ -43,6 +43,7 @@ http {
     js_shared_dict_zone zone=waka:32k timeout=1000s type=number;
     js_shared_dict_zone zone=no_timeout:32k;
     js_shared_dict_zone zone=overflow:32k;
+    js_shared_dict_zone zone=rollback:32k timeout=1000s;
 
     server {
         listen       127.0.0.1:8080;
@@ -128,6 +129,10 @@ http {
             js_content test.ttl;
         }
 
+        location /update_failed {
+            js_content test.update_failed;
+        }
+
         location /zones {
             js_content test.zones;
         }
@@ -344,6 +349,29 @@ $t->write_file('test.js', <<'EOF');
         r.return(200, val === undefined ? 'undefined' : val);
     }
 
+    function update_failed(r) {
+        var dict = ngx.shared.rollback;
+        var failed = false;
+
+        dict.clear();
+        dict.set('target', 'old', 30000);
+
+        try {
+            dict.set('target', 'x'.repeat(dict.capacity * 2));
+
+        } catch (e) {
+            failed = e instanceof SharedMemoryError;
+        }
+
+        dict.set('probe', 'ok');
+
+        var ttl = dict.ttl('target');
+        var valid = ttl !== undefined && ttl > 0;
+
+        r.return(200, `failed:${failed},value:${dict.get('target')},`
+                      + `ttl:${valid}`);
+    }
+
     function zones(r) {
         r.return(200, Object.keys(ngx.shared).sort());
     }
@@ -351,12 +379,12 @@ $t->write_file('test.js', <<'EOF');
     export default { add, capacity, chain, clear, del, free_space, get,
                      has, incr, items, keys, name, njs: test_njs, pop,
                      replace, set, set_clear, size, ttl, zones,
-                     overflow };
+                     overflow, update_failed };
 EOF
 
 $t->try_run('no js_shared_dict_zone');
 
-$t->plan(61);
+$t->plan(64);
 
 ###############################################################################
 
@@ -494,6 +522,17 @@ like(http_get('/size?dict=foo'), qr/size: 0/, 'no of items in foo after clear');
 like(http_get('/set_clear'), qr/size: 0/,
        'no of items in no_timeout after clear');
 
+my $update_failed = http_get('/update_failed');
+like($update_failed, qr/failed:true/, 'failed update throws');
+
+TODO: {
+local $TODO = 'not yet' unless has_version('1.0.3');
+
+like($update_failed, qr/value:old/, 'failed update preserves value');
+like($update_failed, qr/ttl:true/, 'failed update preserves ttl');
+
+}
+
 TODO: {
 local $TODO = 'not yet' unless has_version('0.8.8');