cross modules stream/http shared dict #36

alonbg · 2016-09-07T14:54:52Z

This addresses issue #25 - cross modules shared dict.
Tested (including adding/removing dicts and reload) but tests still need to be added.

I'v covered different ways to do this. Most were either over complicated or/and risky.
This one is quite simple - a module references the other module zones only during post-configuration phase. It works as it relies on the fact (to my best knowledge) that across all pushed dict functions, zone->data is ngx_stream_lua_shdict_ctx_t or ngx_http_lua_shdict_ctx_t which are cast-interchangeable (they are aligned in the attributes that they are referenced to).
The single non compatible attribute is ctx->main_conf but it's only accessed during configuration phase where each module takes care of it's own.
Another big advantage is that it works without any changes to lua_ngx_module. Until http module adopts the change, a dict should be configured in http context for this to work.
This is of course not ideal but it's by far the simplest way.
If possible, it would be best to make these structs fully aligned or even better including a common header file for this. Another TODO is "selectively sharing dicts".

test configuration attached
nginx.txt

agentzh · 2016-09-19T18:55:11Z

src/ngx_stream_lua_shdict.c

@@ -13,6 +13,7 @@
 #include "ngx_stream_lua_shdict.h"
 #include "ngx_stream_lua_util.h"

+extern ngx_module_t ngx_http_lua_module;


Hmm, I don't quite like the fact that the ngx_stream_lua_module always requires ngx_http_lua_module, which is not acceptable when the user does not need the http subsystem at all.

Maybe we should try to detect the presence of ngx_http_lua_module automatically in the build system (i.e., the config file) and fork the logic with C macros.

agentzh · 2016-09-19T18:57:06Z

src/ngx_stream_lua_shdict.c

+        }
+
+        if (&ngx_http_lua_module == shm_zone->tag ||
+            &ngx_stream_lua_module == shm_zone->tag )


Style: || should be at the beginning of the continued line. See other similar places for examples.

Also, we should make constant value on the RHS of the ==.

agentzh · 2016-09-19T19:00:16Z

src/ngx_stream_lua_shdict.c

+        if (&ngx_http_lua_module == shm_zone->tag ||
+            &ngx_stream_lua_module == shm_zone->tag )
+        {
+            zone = ngx_array_push(&all_zones);


This approach is risky since the shdict implementation of ngx_http_lua_module might be different than ngx_stream_lua_module (due to different versions of these modules, for example). It will lead to hard-to-debug issues when these two modules' implementations are incompatible in terms of the shm zone storage layout.

The safest way, IMHO, is to abstract the shm dict implementation out and make both these ngx_xxx_lua modules share it.

@agentzh You mean share the a common header file ? Where would this file reside ?

@alonbg I mean creating a ngx_meta_lua_module and make both ngx_http_lua_module and ngx_stream_lua_module depend on it.

@agentzh Yes this was your original idea :) but to avoid collision [and allow coexistence of old and new] meta's shared dict directive will need to have a different name or stay with the name lua_shared_dict but restricted to the configuration main part (outside both http and stream contexts). Which option do you prefer ?

The risk is still there I believe since the ngx_meta_lua_shdict_ctx_t which would be defined in all three modules might be different.

agentzh

Thanks for looking into this. But this PR still needs quite some work I'm afraid :) Thanks!

agentzh · 2016-09-19T19:01:41Z

src/ngx_stream_lua_socket_tcp.c

+ngx_stream_lua_inject_socket_option_consts(lua_State *L)
+{
+    /* {{{ socket option constants */
+#if (NGX_HAVE_TRANSPARENT_PROXY)


Please, do not include unrelated changes in the PR. Thank you.

agentzh · 2016-09-19T19:04:01Z

@alonbg BTW, the Travis CI testing is broken on this PR, as evidenced below.

alonbg · 2016-09-19T19:43:40Z

@agentzh thanks for taking the time to comment.
The other changes are simply a miserable rebase out of place. I'll rewind.
I agree with your approach. Ill see what I can do.

johnyin123 · 2016-11-02T03:11:24Z

2 or more share dict, when reload nginx, some dict lost .

17 http {
18 lua_shared_dict http_dict 1m;
19 lua_shared_dict http_dict1 1m;
20
21 server {
22 listen 8888;
23
24 location /udp {
25 content_by_lua_block {
26 local http_dict = ngx.shared.http_dict
27 http_dict:set("udp", ngx.var.uri)
28 local stream = http_dict:get("stream")
29 if stream == nil then
30 ngx.say("udp OK")
31 else
32 ngx.say("udp ok " .. stream)
33 end
34 }
35 }
36 }
37 }
38
39 stream {
40 lua_shared_dict stream_dict1 1m;
41 lua_shared_dict stream_dict2 1m;
42
43 server {
44 listen 127.0.0.1:9999;
45 content_by_lua_block {
46 local http_dict = ngx.shared["http_dict"]
47 local http_dict1 = ngx.shared["http_dict1"]
48 http_dict1:set("test", "test")
49 -- ngx.log(ngx.ERR, "get http shdict = " .. http_dict:get("udp"))
50 http_dict:set("stream", "xxxx")
51 }
52 }

telnet localhost 9999
./nginx -s reload
telnet localhost 9999
so the secound http_dict1 nil.

agentzh · 2016-11-03T02:48:47Z

We need a separate ngx_meta_lua_module for this. It's wrong to do such things in either ngx_stream_lua_module or ngx_http_lua_module. Either way is wrong and fragile.

alonbg · 2016-11-08T09:36:03Z

@agentzh, I'm convinced btw. I'll start working on ngx_meta_lua_module when I find the time.
Thanks

bjne · 2017-02-27T18:13:51Z

What is the status on the ngx_meta_lua_module?

thibaultcha · 2020-04-01T21:36:05Z

Here is my alternative proposal to this feature: openresty/meta-lua-nginx-module#76

alonbg force-pushed the cross_modules_shared_dict branch 4 times, most recently from a15851e to 60df5b2 Compare September 8, 2016 07:42

alonbg force-pushed the cross_modules_shared_dict branch from 60df5b2 to a440866 Compare September 18, 2016 08:19

agentzh reviewed Sep 19, 2016

View reviewed changes

agentzh requested changes Sep 19, 2016

View reviewed changes

find http_lua_module by name for shared dict

0d4dda7

alonbg force-pushed the cross_modules_shared_dict branch from 4c78f78 to 0d4dda7 Compare September 21, 2016 10:26

agentzh mentioned this pull request Nov 15, 2016

Can I share some vars to the http block #45

Closed

turbo mentioned this pull request Jul 1, 2019

can we share the shdict between http module and stream module openresty/meta-lua-nginx-module#41

Open

thibaultcha mentioned this pull request Apr 1, 2020

feature: new ngx_meta_lua_module (with multi-subsystems lua_shared_dict support) openresty/meta-lua-nginx-module#76

Draft

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cross modules stream/http shared dict #36

cross modules stream/http shared dict #36

alonbg commented Sep 7, 2016 •

edited

Loading

agentzh Sep 19, 2016

agentzh Sep 19, 2016

agentzh Sep 19, 2016

agentzh Sep 19, 2016

agentzh Sep 19, 2016

agentzh Sep 19, 2016

alonbg Sep 20, 2016

agentzh Sep 20, 2016

alonbg Sep 21, 2016 •

edited

Loading

alonbg Sep 21, 2016 •

edited

Loading

agentzh left a comment

agentzh Sep 19, 2016

agentzh commented Sep 19, 2016

alonbg commented Sep 19, 2016 •

edited

Loading

johnyin123 commented Nov 2, 2016

agentzh commented Nov 3, 2016

alonbg commented Nov 8, 2016

bjne commented Feb 27, 2017

thibaultcha commented Apr 1, 2020

cross modules stream/http shared dict #36

Are you sure you want to change the base?

cross modules stream/http shared dict #36

Conversation

alonbg commented Sep 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alonbg Sep 21, 2016 • edited Loading

Choose a reason for hiding this comment

alonbg Sep 21, 2016 • edited Loading

Choose a reason for hiding this comment

agentzh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agentzh commented Sep 19, 2016

alonbg commented Sep 19, 2016 • edited Loading

johnyin123 commented Nov 2, 2016

agentzh commented Nov 3, 2016

alonbg commented Nov 8, 2016

bjne commented Feb 27, 2017

thibaultcha commented Apr 1, 2020

alonbg commented Sep 7, 2016 •

edited

Loading

alonbg Sep 21, 2016 •

edited

Loading

alonbg Sep 21, 2016 •

edited

Loading

alonbg commented Sep 19, 2016 •

edited

Loading