Skip to content
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.

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

Will shared-memory be implemented or considered? #437

Closed
HermannLizard opened this issue Nov 19, 2021 · 59 comments
Closed

Will shared-memory be implemented or considered? #437

HermannLizard opened this issue Nov 19, 2021 · 59 comments
Assignees
Milestone

Comments

@HermannLizard
Copy link

#431
#354 (comment)

For instance, this can be used in web gray-release solution, totally replace the "redis+nginx+lua" solution, since web maintainers is usually more familiar with JavaScript than lua, like, me. More controllable, more flexible. Sincerely looking forward to it.

@xeioex xeioex added the feature label Nov 19, 2021
@xeioex
Copy link
Contributor

xeioex commented Nov 19, 2021

Hi @HermannLizard,

We are planning to open source keyval module for this.

@chs97
Copy link

chs97 commented Nov 10, 2022

@xeioex
Are there any updates

@raoajayy
Copy link

@xeioex Any timeline to open source keyval module

@xeioex
Copy link
Contributor

xeioex commented Apr 6, 2023

Before keyval module is release, I plan to add a shared dictionary to njs.

@xeioex
Copy link
Contributor

xeioex commented Jun 9, 2023

Hello everyone,

@HermannLizard, @osokin, @drsm, @ivanitskiy, @crasyangel, @FlorianSW, @7rulnik, @bfuvx, @cxpcyv, @jirutka and others feel free to test and give feedback for upcoming shared dictionary feature.

https://gist.github.com/xeioex/b61de2bba239cc84bf7c459966367ddf

Modules: introduced js_shared_dict_zone directive.

The directive allows to declared a dictionary that is shared among the
working processes. A dictonary expects strings as keys. It stores
string or number as values. The value type is declared using
type= argument of the direcive. The default type is string.

For example:
    nginx.conf:
        # Declares a shared dictionary of strings of size 1 Mb, that
        # removes key-value after 60 seconds of inactivity.
        js_shared_dict_zone zone=foo:1M timeout=60s;
        # Declares a shared dictionary of strings of size 512Kb, that
        # forsibly remove oldest key-value pairs when memory is not enough.
        js_shared_dict_zone zone=bar:512K timeout=30s evict;
        # Declares a permanent number shared dictionary of size 32Kb.
        js_shared_dict_zone zone=num:32k type=number;

    test.js:
        function get(r) {
            r.return(200, ngx.shared.foo.get(r.args.key));

        function set(r) {
            r.return(200, ngx.shared.foo.set(r.args.key, r.args.value));
        }

        function delete(r) {
            r.return(200, ngx.shared.bar.delete(r.args.key));
        }

        function increment(r) {
            r.return(200, ngx.shared.num.incr(r.args.key, 1));
        }
@drsm
Copy link
Contributor

drsm commented Jun 10, 2023

@hongzhidao
Copy link
Contributor

hongzhidao commented Jun 12, 2023

Hi Dmitry,

  1. About the commit log.
https://gist.github.com/xeioex/357e64b871608cae08f8fc7fb184b151#file-shared_dict-patch-L9
s/The directives allows/The directive allows/

    test.js:
        function get(r) {
            r.return(200, ngx.shared.foo.get(r.args.key));
       // missed } here 

        function set(r) {
            r.return(200, ngx.shared.foo.set(r.args.key, r.args.value));
        }
  1. What's the intention of the usage?
    js_shared_dict_zone zone=bar:512K timeout=30s evict;
    js_shared_dict_zone zone=baz:512K timeout=30s evict;

    var dict = ngx.shared.baz;
    dict.set("foo", "baz");

    r.return(200, ngx.shared.get("foo"));

The ngx.shared.get is the same as ngx.shared.baz.get, since the last zone was regarded as the default shared dict. Is it your intention? Is it because of the convenience for users?

  1. Handing the case of empty value.
    a. missed a test on it.
    b. the size of value->length is 0 for both empty value and delete operation, so using value->start == NULL for the delete operation.
diff -r 3280c24bba68 nginx/ngx_shared_dict.c
--- a/nginx/ngx_shared_dict.c   Fri Jun 09 00:08:46 2023 -0700
+++ b/nginx/ngx_shared_dict.c   Mon Jun 12 18:22:21 2023 +0800
@@ -313,7 +313,7 @@
     node = ngx_js_dict_lookup(dict, key);

     if (node == NULL) {
-        if (value->length == 0) {
+        if (value->start == NULL) {
             ngx_rwlock_unlock(&dict->sh->rwlock);
             return NGX_OK;
         }
@@ -325,7 +325,7 @@
             return NGX_ERROR;
         }

-    } else if (value->length == 0) {
+    } else if (value->start == NULL) {
         ngx_js_dict_delete(dict, node);

     } else {
  1. Just an idea to simplify related allocation and free.
diff -r 3280c24bba68 nginx/ngx_shared_dict.c
--- a/nginx/ngx_shared_dict.c   Fri Jun 09 00:08:46 2023 -0700
+++ b/nginx/ngx_shared_dict.c   Mon Jun 12 18:33:05 2023 +0800
@@ -45,18 +45,20 @@
 ngx_js_dict_add(ngx_js_dict_t *dict, njs_str_t *key, njs_str_t *value,
     ngx_msec_t expire)
 {
+    size_t               n;
     uint32_t             hash;
     ngx_js_dict_node_t  *node;

     hash = ngx_crc32_long(key->start, key->length);

-    node = ngx_slab_alloc_locked(dict->shpool, sizeof(ngx_js_dict_node_t));
+    n = sizeof(ngx_js_dict_node_t) + key->length;
+
+    node = ngx_slab_alloc_locked(dict->shpool, n);
     if (node == NULL) {
         if (dict->evict) {
             ngx_js_dict_evict(dict, 16);

-            node = ngx_slab_alloc_locked(dict->shpool,
-                                         sizeof(ngx_js_dict_node_t));
+            node = ngx_slab_alloc_locked(dict->shpool, n);
         }

         if (node == NULL) {
@@ -64,20 +66,7 @@
         }
     }

-    node->sn.str.data = ngx_slab_alloc_locked(dict->shpool, key->length);
-    if (node->sn.str.data == NULL) {
-        if (dict->evict) {
-            ngx_js_dict_evict(dict, 16);
-
-            node->sn.str.data = ngx_slab_alloc_locked(dict->shpool,
-                                                   sizeof(ngx_js_dict_node_t));
-        }
-
-        if (node->sn.str.data == NULL) {
-            ngx_slab_free_locked(dict->shpool, node);
-            return NGX_ERROR;
-        }
-    }
+    node->sn.str.data = (u_char *) node + key->len;

     node->value.data = ngx_slab_alloc_locked(dict->shpool, value->length);
     if (node->value.data == NULL) {
@@ -85,11 +74,10 @@
             ngx_js_dict_evict(dict, 16);

             node->value.data = ngx_slab_alloc_locked(dict->shpool,
-                                                   sizeof(ngx_js_dict_node_t));
+                                                     value->length);
         }

         if (node->value.data == NULL) {
-            ngx_slab_free_locked(dict->shpool, node->sn.str.data);
             ngx_slab_free_locked(dict->shpool, node);
             return NGX_ERROR;
         }
@@ -160,7 +148,6 @@
     ngx_rbtree_delete(&dict->sh->rbtree, (ngx_rbtree_node_t *) node);

     ngx_slab_free_locked(dict->shpool, node->value.data);
-    ngx_slab_free_locked(dict->shpool, node->sn.str.data);
     ngx_slab_free_locked(dict->shpool, node);
 }

@@ -251,7 +238,6 @@
         ngx_rbtree_delete(&dict->sh->rbtree, (ngx_rbtree_node_t *) node);

         ngx_slab_free_locked(dict->shpool, node->value.data);
-        ngx_slab_free_locked(dict->shpool, node->sn.str.data);
         ngx_slab_free_locked(dict->shpool, node);
     }
 }
@@ -288,7 +274,6 @@
         ngx_rbtree_delete(&dict->sh->rbtree, (ngx_rbtree_node_t *) node);

         ngx_slab_free_locked(dict->shpool, node->value.data);
-        ngx_slab_free_locked(dict->shpool, node->sn.str.data);
         ngx_slab_free_locked(dict->shpool, node);
     }
 }
@xeioex
Copy link
Contributor

xeioex commented Jun 14, 2023

@hongzhidao

Thanks for reviewing the code, it was helpful.

What's the intention of the usage?

Yes, the original intention was to have a shortcut when there is only one shared dictionary.
But after some thinking I decided to remove it, to simplify the code and not to confuse the users.

Handing the case of empty value.

Thanks, fixed the test case. Also reworked the deletion, so it is always explicit and empty strings are also allowed now.

Just an idea to simplify related allocation and free.

That does not work when we want to replace the values, we want to reallocate only the value part.

@hongzhidao
Copy link
Contributor

@xeioex You are welcome.

That does not work when we want to replace the values, we want to reallocate only the value part.

You are right, but take a look again, I only combine the node structure with the key.

-    node = ngx_slab_alloc_locked(dict->shpool, sizeof(ngx_js_dict_node_t));
+    n = sizeof(ngx_js_dict_node_t) + key->length;
@xeioex
Copy link
Contributor

xeioex commented Jun 14, 2023

@drsm, @hongzhidao

A topic for discussion

What ngx.shared.foo.set() should do when

  1. memory is not enough

Should it return a boolean value signifying success/failure (as of now), or it should throw an exception?

What would be intuitive and convenient for users?

For the context, in the first patch the only reason of failure was non enough memory, and a boolean was sufficient.
But I am working now on must_exist | should_not_exist flags. As a result there will be at least two different reasons for a falure of set().

@hongzhidao
Copy link
Contributor

My thoughts are:

  1. It would be useful to log error when it happens.
  2. I found, for now, the only case to return failure for set() is the shared memory is not enough,
    but there might more cases like "too long length for key", or "empty key".
    So I prefer to throw an exception to indicate not enough memory happened.

BTW, do we need to handle the empty key for both set and delete?

@xeioex
Copy link
Contributor

xeioex commented Jun 14, 2023

@hongzhidao

BTW, do we need to handle the empty key for both set and delete?

No, we do not. I reworked the part with deletion, so it is done explicitly.

@xeioex
Copy link
Contributor

xeioex commented Jun 14, 2023

@jirutka
Copy link
Contributor

jirutka commented Jun 14, 2023

# Declares a shared dictionary of strings of size 1 Mb, that
# removes key-value after 60 seconds of inactivity.
js_shared_dict_zone zone=foo:1M timeout=60s;

I find timeout a bit confusing, I think that ttl would be more accurate. When I see timeout, I think about network timeout, operation timeout, …, an interval after which something is aborted as a failure. On the other hand, timeout in the meaning of TTL is already used in keyval_zone, so it’s probably better to be consistent with that.

What ngx.shared.foo.set() should do when memory is not enough
Should it return a boolean value signifying success/failure (as of now), or it should throw an exception?

It should definitely throw an exception for the reasons already mentioned. Also, returning a boolean to indicate a failure is not a good practice in general (except in C…) – it often leads to silent/unhandled errors because devs forget or don’t care to handle it.

But I am working now on must_exist | should_not_exist flags.

Can you please elaborate on this?

@drsm
Copy link
Contributor

drsm commented Jun 14, 2023

@xeioex

I think we should throw on allocation failure. So one can distinguish a case of existent|nonexistent value from a low memory conditions.

BTW,

-    items(): number;
+   readonly size: number; // like in Map, Set

-    freeSpace(): number;
+   readonly freeSpace: number;

Also, it would be nice to have a has method, to reduce a boilerplate in a case of shared set.

@jirutka
Copy link
Contributor

jirutka commented Jun 14, 2023

items(): number;
freeSpace(): number;

As @drm already said, readonly size: number and readonly freeSpace: number would be better.

> get(id: string): NgxSharedDictValue;

What will this do if an entry with the specified id does not exist? To be consistent with Object, Map, Set… it should return undefined, so the return type should be NgxSharedDictValue | undefined.

> /**
>  * Sets a value by key.
>  * @param id is a string key.
>  * @param value is a string or number value.
>  * @param flags is in optional number value with flags.
>  * 1 - the value must exist in the dictionary (the same as replace()).
>  * 2 - the value must not exist in the dictionary (the same as add()).
>  * 0 is the default value (no flags).
>  * @returns true if the value has been successfully set,
>  * and false otherwise.
>  */
> set(id: string, value:NgxSharedDictValue, flags?:number): boolean;

I’d prefer making set() consistent with Map.prototype.set():

  • drop flags – they’re redundant, you have separate methods (add and replace) doing the same thing.
  • return “this” instead of boolean to allow chaining
incr(id: string, delta: number, init?: number): number;

What will this method do if called on a dictionary of type string?

Can you also consider adding the following methods?

  • has(id: string): boolean – returns a boolean indicating whether an element with the specified key exists or not
  • keys(): IterableIterator<string> (or keys(): string[]) – returns an interator that contains the keys for each entry in the dict
  • clear(): void – removes all entries from the dict
@drsm
Copy link
Contributor

drsm commented Jun 14, 2023

@xeioex
@jirutka

How about:

readonly name: string
readonly capacity: number;
readonly freeSpace: number;
incr(id: string, delta: number, init: number): number;

// mimic JS Map
readony size: number;
set(id: string, value:NgxSharedDictValue): NgxSharedDict;
delete(id: string): boolean;
get(id: string): NgxSharedDictValue | undefined;
has(id: string): boolean;
keys(): string[];
clear(): void;

// SQL like setters
insert(id: string, value:NgxSharedDictValue): boolean;
update(id: string, value:NgxSharedDictValue): boolean;
@xeioex
Copy link
Contributor

xeioex commented Jun 14, 2023

@jirutka

As @drm already said, readonly size: number and readonly freeSpace: number would be better

Originally, when making capacity as property and freeSpace() and items(), the distinction was that the first is a constant and the latter are variables. Also the methods signify for the user that accessing it is not cost-free. As for example with items() we in order to count the items block the whole tree.

@jirutka
Copy link
Contributor

jirutka commented Jun 14, 2023

Originally, when making capacity as property and freeSpace() and items(), the distinction was that the first is a constant and the latter are variables.

This makes sense. However, I find items() confusing, I would expect it to return a list or iterator over the items in the dictionary. Naming it size() would be better.

@xeioex
Copy link
Contributor

xeioex commented Jun 14, 2023

@jirutka, @drsm

On the other hand, timeout in the meaning of TTL is already used in keyval_zone, so it’s probably better to be consistent with that.

yes, that was my intention to be consistent with keyval.

But I am working now on must_exist | should_not_exist flags.

Can you please elaborate on this?

That was about flags for the set() method.

drop flags – they’re redundant, you have separate methods (add and replace) doing the same thing.

I was thinking about allowing a user to choose programmatically the behaviour of set(), but I agree it is probably redundant to add() and replace().

incr(id: string, delta: number, init?: number): number;
What will this method do if called on a dictionary of type string?

It throws an exception now.

Can you also consider adding the following methods?

Yes, I plan to add them. I am thinking about

readonly name: string;
readonly capacity: number;
freeSpace(): number;  // method because it involves calculation
incr(id: string, delta: number, init: number): number;

size(): number;  // method because it involves counting
set(id: string, value:NgxSharedDictValue): NgxSharedDict;
delete(id: string): boolean;
get(id: string): NgxSharedDictValue | undefined;
has(id: string): boolean;
keys(max_count?:number): string[]; // optional limit for performance
clear(): void;
@hongzhidao
Copy link
Contributor

delete(id: string): boolean;

What does this return value represent?

@hongzhidao
Copy link
Contributor

hongzhidao commented Jun 15, 2023

@xeioex

@hongzhidao here is the latest version
https://gist.github.com/xeioex/b61de2bba239cc84bf7c459966367ddf

I reworked it. But since you plan to simplify the APIs related to "set/add/replace", the patch is to show where could be reworked.
https://gist.github.com/hongzhidao/1eb725273389c1982adc4af093079847

  1. For the APIs ngx_js_dict_{name}(), dict is more readable than shm_zone as its first parameter.
  2. Introduced something like ngx_js_dict_alloc/free() as a helper to simplify related allocation and release.
  3. Tiny fixes.
@xeioex
Copy link
Contributor

xeioex commented Jun 15, 2023

@hongzhidao

What does this return value represent?

false will be returned when a value to delete was absent.

@hongzhidao
Copy link
Contributor

@hongzhidao

What does this return value represent?

false will be returned when a value to delete was absent.

I thought this was your intention, and it seems you missed something here.

    ngx_js_dict_delete(shm_zone->data, &key);  /* should do something on it */

    njs_value_boolean_set(retval, 1);
@xeioex
Copy link
Contributor

xeioex commented Jun 15, 2023

@drsm, @jirutka, @hongzhidao

https://gist.github.com/xeioex/64e78d037bedb491bd1ebd3c48a5f78f

Here is the version the incorporates recent suggests.

  1. clear(), has(), keys() were added
  2. items() renamed to size()
  3. add(), set(), replace() now throw exception if memory is not enough and return this
  4. code improvements by @hongzhidao were merges with minor changes
@xeioex
Copy link
Contributor

xeioex commented Jun 15, 2023

@hongzhidao

shared_dict v4
https://gist.github.com/xeioex/6b7c18c969dfa0189e4e2d9ab6d4a4af

applied improvement from here.

@hongzhidao, @jirutka

Do you have any opinion on drsm comment what should be the retval of add() and replace().

@hongzhidao
Copy link
Contributor

@drsm
Copy link
Contributor

drsm commented Jun 16, 2023

@xeioex

But I still want to see the usefulness of such chaining construction for set() method in practice.

I don't have any arguments for chaining in set() other than POLA.

But, for add() or replace() it will made error control painful.

dumb PoC:

try {
    sid = null;

    // just try to do the same with exceptions
    if (!njx.shared.user.add(uid, Date.now())) {
        return processSuspiciousLogin(r, uid);
    }
    
    sid = generateSid();
    
    njx.shared.session.set(sid, uid);
    
    // ...
    return true;
    
} catch (e) {
    if (e instanceof MemoryError) {
        // it would be nice to have SharedMemoryError, 'cause MemoryError may came from a different source
        njx.shared.user.remove(uid);
        r.log(`unable to login ${uid}, consider increasing ${sid ? 'session' : 'user'} shared memory zone`);
    
    } else {
        // something unexpected
    }
    
    return false;
}
@xeioex
Copy link
Contributor

xeioex commented Jun 16, 2023

@drsm

But, for add() or replace() it will made error control painful.

Thanks, that was a good example. I will update the retval for add() and replace().

@hongzhidao
Copy link
Contributor

@xeioex

Great job, no issue found. And here's a small rework. Others look good.
https://gist.github.com/hongzhidao/48a5be0e82a5c1a01208d1012f83ba3c

Introduced a mistake here.
https://gist.github.com/hongzhidao/48a5be0e82a5c1a01208d1012f83ba3c#file-shared_dict4-patch-L60

Updated with:

+    njs_value_boolean_set(retval, node != NULL);
     ngx_rwlock_unlock(&dict->sh->rwlock);
-    njs_value_boolean_set(retval, has);
@xeioex
Copy link
Contributor

xeioex commented Jun 17, 2023

@hongzhidao

Introduced a mistake here

Yes, I already fixed it.

@jirutka
Copy link
Contributor

jirutka commented Jun 18, 2023

Do you have any opinion on drsm comment what should be the retval of add() and replace().

I agree with @drsm. Since add should set a value by key only if the key does not exist yet, it should return true when the key didn’t exist and so the value was set, false otherwise. replace should replace a value by key only if the key exists, so it should return true when the key exists and so the value was replaced, false otherwise. In all cases, out-of-memory should be indicated by an exception.

The question is, what exception type? It’ll be better to “subclass” Error (or some other error type), so it can be disguised from other error cases. @drsm suggested SharedMemoryError, I would choose something more specific, e.g. OutOfSharedMemoryError?

@jirutka
Copy link
Contributor

jirutka commented Jun 18, 2023

I also have an update of the types, but I cannot finish it now, I’ll add it later today.

@jirutka
Copy link
Contributor

jirutka commented Jun 19, 2023

I rewrote the type declaration for NgxSharedDict to:

  • use generic for the value type so users can (optionally) assert a particular shared dictionary to its value type (const mydict = shared.mydict as NgxSharedDict<number>),
  • forbid incr() method in NgxSharedDict<string>,
  • rewrite the comments to be more informative (especially about errors) and consistent with existing types,
  • highlight method arguments in the comment texts,
  • declare the suggested OutOfSharedMemoryError type.
/**
 * This Error object is thrown when adding an item to a shared dictionary
 * that does not have enough free space.
 * @since 0.8.0
 */
declare class OutOfSharedMemoryError extends Error {}

/**
 * Interface of a dictionary shared among the working processes.
 * It can store either `string` or `number` values which is specified when
 * declaring the zone.
 * 
 * @type {V} The type of stored values.
 * @since 0.8.0
 */
interface NgxSharedDict<V extends string | number = string | number> {
    /**
     * The capacity of this shared dictionary in bytes.
     */
    readonly capacity: number;
    /**
     * The name of this shared dictionary.
     */
    readonly name: string;

    /**
     * Sets the `value` for the specified `key` in the dictionary only if the
     * `key` does not exist yet.
     * 
     * @param key The key of the item to add.
     * @param value The value of the item to add.
     * @returns `true` if the value has been added successfully, `false`
     *   if the `key` already exists in this dictionary.
     * @throws {OutOfSharedMemoryError} if there's not enough free space in this
     *   dictionary.
     * @throws {TypeError} if the `value` is of a different type than expected
     *   by this dictionary.
     */
    add(key: string, value: V): boolean;
    /**
     * Removes all items from this dictionary.
     */
    clear(): void;
    /**
     * Removes the item associated with the specified `key` from the dictionary.
     * 
     * @param key The key of the item to remove.
     * @returns `true` if the item in the dictionary existed and has been
     *   removed, `false` otherwise.
     */
    delete(key: string): boolean;
    /**
     * Increments the value associated with the `key` by the given `delta`.
     * If the `key` doesn't exist, the item will be initialized to `init`.
     * 
     * **Important:** This method can be used only if the dictionary was
     * declared with `type=number`!
     * 
     * @param key is a string key.
     * @param delta The number to increment/decrement the value by.
     * @param init The number to initialize the item with if it didn't exist
     *   (default is `0`).
     * @returns The new value.
     * @throws {OutOfSharedMemoryError} if there's not enough free space in this
     *   dictionary.
     * @throws {TypeError} if this dictionary does not expect numbers.
     */
    incr: V extends number
      ? (key: string, delta: V, init?: number) => number
      : never;
    /**
     * @returns The free page size in bytes.
     *   Note that even if the free page is zero the dictionary may still accept
     *   new values if there is enough space in the occupied pages.
     */
    freeSpace(): number;
    /**
     * @param key The key of the item to retrieve.
     * @returns The value associated with the `key`, or `undefined` if there
     *   is none.
     */
    get(key: string): V | undefined;
    /**
     * @param key The key to search for.
     * @returns `true` if an item with the specified `key` exists, `false`
     *   otherwise.
     */
    has(key: string): boolean;
    /**
     * @param maxCount The maximum number of keys to retrieve (default is 1024).
     * @returns An array of the dictionary keys.
     */
    keys(maxCount?: number): string[];
    /**
     * Sets the `value` for the specified `key` in the dictionary only if the
     * `key` already exists.
     * 
     * @param key The key of the item to replace.
     * @param value The new value of the item.
     * @returns `true` if the value has been replaced successfully, `false`
     *   if the key doesn't exist in this dictionary.
     * @throws {OutOfSharedMemoryError} if there's not enough free space in this
     *   dictionary.
     * @throws {TypeError} if the `value` is of a different type than expected
     *   by this dictionary.
     */
    replace(key: string, value: V): boolean;
    /**
     * Sets the `value` for the specified `key` in the dictionary.
     *  
     * @param key The key of the item to set.
     * @param value The value of the item to set.
     * @returns This dictionary (for method chaining).
     * @throws {OutOfSharedMemoryError} if there's not enough free space in this
     *   dictionary.
     * @throws {TypeError} if the `value` is of a different type than expected
     *   by this dictionary.
     */
    set(key: string, value: V): this;
    /**
     * @returns The number of items in this shared dictionary.
     */
    size(): number;
}

interface NgxGlobalShared {
    /**
     * Shared dictionaries.
     * @since 0.8.0
     */
    readonly [name: string]: NgxSharedDict;
}
@jirutka
Copy link
Contributor

jirutka commented Jun 19, 2023

if (dict->type == NGX_JS_DICT_TYPE_STRING) {
    if (!njs_value_is_string(value)) {
        njs_vm_error(vm, "string value is expected");
        return NJS_ERROR;
    }
} else {
    if (!njs_value_is_number(value)) {
        njs_vm_error(vm, "number value is expected");
        return NJS_ERROR;
    }
}

This throws Error, right? It would be better to throw TypeError instead.

@jirutka
Copy link
Contributor

jirutka commented Jun 19, 2023

It would also be useful to have a method that will atomically return and delete a value specified by the key. We can either change the delete() method or add another method, e.g. pop() – if we wanna stick to the Map interface and/or if returning the deleted object have a measurable performance impact.

@xeioex
Copy link
Contributor

xeioex commented Jun 21, 2023

Hi @jirutka and @drsm,

Thank you for good suggestions. I plan to implement them.

Currently I am working on allowing external modules (like the shared dict code in the patch) to create its own exception types cheaply.

@xeioex xeioex added this to the 0.8.0 milestone Jun 30, 2023
@xeioex
Copy link
Contributor

xeioex commented Jul 1, 2023

@jirutka, @hongzhidao, @drsm

Here is the final shared dict patch: https://gist.github.com/xeioex/1429e987b0f585ef9eb599b49fcf5a5c

It includes

  1. SharedMemoryError
  2. pop() method
  3. TypeError exceptions for unexpected values
  4. TypeScript updates from @jirutka
@jirutka
Copy link
Contributor

jirutka commented Jul 1, 2023

- pop(key: string): boolean;
+ pop(key: string): V | undefined;
@hongzhidao
Copy link
Contributor

@xeioex

Here is the final shared dict patch: https://gist.github.com/xeioex/1429e987b0f585ef9eb599b49fcf5a5c

Looks good to me.

@jirutka
Copy link
Contributor

jirutka commented Jul 3, 2023

- * @type {V} The type of stored values.
+ * @template V The type of stored values.
@xeioex
Copy link
Contributor

xeioex commented Jul 3, 2023

@drsm, @jirutka, @hongzhidao

Thanks guys, committed. I appreciate your contribution.

@xiaoxiangmoe
Copy link

xiaoxiangmoe commented Jul 10, 2023

Hi @xeioex, do you know when will https://github.com/nginxinc/docker-nginx update njs version to 0.8.0?

@xeioex
Copy link
Contributor

xeioex commented Jul 10, 2023

Hi @xiaoxiangmoe,

I think @thresheek, may answer this question.

@jirutka
Copy link
Contributor

jirutka commented Jul 10, 2023

do you know when will https://github.com/nginxinc/docker-nginx update njs version to 0.8.0?

Or you can just use a bare image with Alpine Linux Edge and install nginx, nginx-mod-http-js and any other modules you need. We also package dozens of third-party modules. ;)

@xeioex
Copy link
Contributor

xeioex commented Jul 12, 2023

@xiaoxiangmoe the image will be updated with new nginx release.

@rikatz
Copy link

rikatz commented Jul 16, 2023

Hey folks,

I think there's just a small mistake on docs, apparently "delete" is a reserved word and cannot be used as a function name?

When using it as

function delete(r) {
        r.return(200, ngx.shared.certs.delete(r.args.key));
}

it gives me Unexpected token "delete" while if the function is called "del(r)" it works fine.

@jirutka
Copy link
Contributor

jirutka commented Jul 16, 2023

it gives me Unexpected token "delete" while if the function is called "del(r)" it works fine.

So don’t name it delete… Functions can be named delete, but you cannot declare it like this. For example, Foo.prototype.delete = function() { } is allowed.

@rikatz
Copy link

rikatz commented Jul 16, 2023

I just copied the example from docs, that’s why I’ve commented it :)

@jirutka
Copy link
Contributor

jirutka commented Jul 16, 2023

Aha, I see now, you’re right.

@xeioex, where can I find repository for https://nginx.org/en/docs/njs/ ?

@xiaoxiangmoe
Copy link

xiaoxiangmoe commented Jul 17, 2023

https://nginx.org/en/docs/http/ngx_http_js_module.html#js_shared_dict_zone

js_shared_dict_zone

Sets the name and size of the shared memory zone that keeps the key-value dictionary shared between worker processes.

Is there anything that will define a shared variable between all requests for each single worker process?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment