-
Notifications
You must be signed in to change notification settings - Fork 128
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
Comments
Hi @HermannLizard, We are planning to open source keyval module for this. |
@xeioex |
Before keyval module is release, I plan to add a shared dictionary to njs. |
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
|
Hi Dmitry,
The
|
Thanks for reviewing the code, it was helpful.
Yes, the original intention was to have a shortcut when there is only one shared dictionary.
Thanks, fixed the test case. Also reworked the deletion, so it is always explicit and empty strings are also allowed now.
That does not work when we want to replace the values, we want to reallocate only the value part. |
@xeioex You are welcome.
You are right, but take a look again, I only combine the node structure with the key.
|
A topic for discussion What
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. |
My thoughts are:
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. |
@hongzhidao here is the latest version https://gist.github.com/xeioex/b61de2bba239cc84bf7c459966367ddf |
I find
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.
Can you please elaborate on this? |
I think we should throw on allocation failure. So one can distinguish a case of existent|nonexistent value from a low memory conditions. BTW,
Also, it would be nice to have a |
As @drm already said,
What will this do if an entry with the specified id does not exist? To be consistent with Object, Map, Set… it should return
I’d prefer making
What will this method do if called on a dictionary of type string? Can you also consider adding the following methods?
|
How about:
|
Originally, when making |
This makes sense. However, I find |
yes, that was my intention to be consistent with keyval.
That was about flags for the set() method.
I was thinking about allowing a user to choose programmatically the behaviour of
It throws an exception now.
Yes, I plan to add them. I am thinking about
|
What does this return value represent? |
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.
|
false will be returned when a value to delete was absent. |
I thought this was your intention, and it seems you missed something here.
|
https://gist.github.com/xeioex/64e78d037bedb491bd1ebd3c48a5f78f Here is the version the incorporates recent suggests.
|
shared_dict v4 applied improvement from here. Do you have any opinion on drsm comment what should be the retval of |
Great job, no issue found. And here's a small rework. Others look good. |
I don't have any arguments for chaining in But, for 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;
} |
Thanks, that was a good example. I will update the retval for |
Introduced a mistake here. Updated with:
|
Yes, I already fixed it. |
I agree with @drsm. Since The question is, what exception type? It’ll be better to “subclass” |
I also have an update of the types, but I cannot finish it now, I’ll add it later today. |
I rewrote the type declaration for
/**
* 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;
} |
This throws |
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 |
Here is the final shared dict patch: https://gist.github.com/xeioex/1429e987b0f585ef9eb599b49fcf5a5c It includes
|
- pop(key: string): boolean;
+ pop(key: string): V | undefined; |
Looks good to me. |
- * @type {V} The type of stored values.
+ * @template V The type of stored values. |
Thanks guys, committed. I appreciate your contribution. |
Hi @xeioex, do you know when will https://github.com/nginxinc/docker-nginx update njs version to 0.8.0? |
Hi @xiaoxiangmoe, I think @thresheek, may answer this question. |
Or you can just use a bare image with Alpine Linux Edge and install |
@xiaoxiangmoe the image will be updated with new nginx release. |
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
it gives me |
So don’t name it |
I just copied the example from docs, that’s why I’ve commented it :) |
Aha, I see now, you’re right. @xeioex, where can I find repository for https://nginx.org/en/docs/njs/ ? |
https://nginx.org/en/docs/http/ngx_http_js_module.html#js_shared_dict_zone
Is there anything that will define a shared variable between all requests for each single worker process? |
#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.
The text was updated successfully, but these errors were encountered: