nginx / njs

A subset of JavaScript language to use in nginx
http://nginx.org/en/docs/njs/
BSD 2-Clause "Simplified" License
841 stars 116 forks source link

Modules: shared dictionary add, set, incr methods timeout argument. #726

Closed jo-carter closed 3 weeks ago

jo-carter commented 1 month ago

The optional timeout argument overrides the timeout specified with the shared_dict_zone directive for the effected key and operation only. The timeout is specified in milliseconds.

This is useful when the majority of keys are expected to require unique timeouts.

Proposed changes

Describe the use case and detail of the change. If this PR addresses an issue on GitHub, make sure to include a link to that issue using one of the supported keywords here in this description (not in the title of the PR).

Checklist

Before creating a PR, run through this checklist and mark each as complete:

jo-carter commented 1 month ago

Keyval_zone has the same feature for updates over the N+ API, for reference: https://nginx.org/en/docs/http/ngx_http_api_module.html#def_nginx_http_keyval_zone_post_patch

jo-carter commented 3 weeks ago

@xeioex Thank you for the review and patches provided. 100% agreed on all your points regarding style, formatting, tests. and the commit message.

I made two small adjustments to the patches you provided:

  1. For consistency I adjusted the shortened variable name for njs_value_t for timeout argument to match in both _njs_js_ext_shared_dictset and _njs_js_ext_shared_dictincr - both are now timeo.
  2. I've re-arranged order of new tests to make false outcome less likely(similar to "bar keys after a delay" test). Probably an explicit sleep would be better, however ms level sleeps seem to require importing non-core modules in perl...

Let me know what you think of these adjustments.

jo-carter commented 3 weeks ago

@xeioex Thanks again :)