add exit_worker_by* feature #927

lixianliang · 2016-12-13T02:17:04Z

I hereby granted the copyright of the changes in this pull request
to the authors of this lua-nginx-module project.
你好，我在使用ngx_lua的场景中是一些图片计算或GPU（ai）计算，在init_worker_by阶段会初始GPU或图片Caffe资源，当进行reload的时候，由于worker process exit没有去清理GPU或者Caffe内部的资源，会导致worker出现core，所以我针对此请求再ngx_lua添加了exit_worker_by阶段。希望eixt_worker_by能合并到master。我相信其他的使用者也有可能会用此功能。
我们的技术堆栈为 nginx + lua + （imagexxx.so | aixxx.so ==> 会封转成lua扩展）

agentzh · 2016-12-13T02:21:00Z

@lixianliang Thank you for your contribution! But please use English instead of Chinese. Thanks for your cooperation!

thibaultcha · 2016-12-13T02:22:44Z

src/ngx_http_lua_module.c

+      0,
+      (void *) ngx_http_lua_exit_worker_by_inline },
+
+    { ngx_string("exit_worker_by_lua"),


Nice work!

I believe the newly added Lua handlers should only use the by_block form though, as per the ssl_certificate_by_lua_block demonstrates?

I believe you would also need to update ngx_http_lua_{phase | common | util}.c to introduce this new phase context? As well as setting the context itself? I could be wrong.

I believe it will also be requested of you that you introduce tests for this feature.

i reference to init_worker_by*, there have three method(init_worker_by_lua init_worker_by_lua_block init_worker_by_lua_file)

Yes, the init_worker phase was introduced before the *_by_lua_block form and as of today, this module still maintains the older *_by_lua form for backwards compatibility reasons. However, newly introduced phases (as the ssl_certificate one) are only implemented in the *_by_lua_block form.

Hey, on the bright side of things, it's less work for you ;)

ok,thank you. i will add this.

lixianliang · 2016-12-13T02:25:52Z

sorry，my english very pool，i need help。latter i will push request again。

src/ngx_http_lua_directive.c

agentzh · 2016-12-13T02:22:24Z

src/ngx_http_lua_exitworkerby.c

@@ -0,0 +1,60 @@
+
+/*
+ * Copyright (C) Xianliang Li


For this patch to merge, you have to grant your copyright to the authors of this module. So please remove this copyright notice comment.

agentzh · 2016-12-13T02:23:16Z

src/ngx_http_lua_module.c

+      0,
+      (void *) ngx_http_lua_exit_worker_by_inline },
+
+    { ngx_string("exit_worker_by_lua"),


Newly added directives should only provide the *_by_lua_block and *_by_lua_file forms. Please remove this exit_worker_by_lua directive. Thank you.

enen，i will correction this。

agentzh · 2016-12-13T02:24:17Z

src/ngx_http_lua_directive.c

+
+        lmcf->exit_worker_src.data = name;
+        lmcf->exit_worker_src.len = ngx_strlen(name);
+    } else {


Style: we need a blank line before the } else { line. See other similar places in the existing code base. Thank you!

agentzh · 2016-12-13T02:31:46Z

Some other suggestions:

Please add corresponding test cases for this new feature to the existing test suite. We always require new tests to cover every new feature :)
Please add some corresponding docs for this new feature to the doc/HttpLuaModule.wiki file and update README.markdown with the ngx-releng tool accordingly.
The ngx-releng tool reports some coding style issues in your patch. Please fix them accordingly.

Thanks!

agentzh · 2016-12-13T02:34:04Z

@thibaultcha Thanks for your review! Next time I'll just wait :)

thibaultcha · 2016-12-13T02:37:36Z

@agentzh no problem, I think both of use jumped on this at the same time :)

side note: quite excited to see this as I was thinking about making the same contribution recently :) Just happy to help.

thibaultcha · 2016-12-13T05:55:30Z

@lixianliang At some point, you will also be asked to write the documentation for this new phase handler. If you don't feel comfortable enough for providing an English version, I'd be happy to help you.

membphis · 2017-01-02T06:43:35Z

src/ngx_http_lua_exitworkerby.h

+ngx_int_t ngx_http_lua_exit_worker_by_file(ngx_log_t *log,
+    ngx_http_lua_main_conf_t *lmcf, lua_State *L);
+
+void      ngx_http_lua_exit_worker(ngx_cycle_t *cycle);


it only needs one space between void and ngx_http_lua_exit_worker

lixianliang · 2017-01-03T02:47:43Z

Hi, I want to add exit_worker feature, This function runs the specified Lua code upon every Nginx worker process's exiting when the master process is enabled. This hook is often used to release resources when init_worker_by* create resources, To prevent the worker exit abnormal

membphis · 2017-01-03T03:30:31Z

src/ngx_http_lua_exitworkerby.c

+        || lmcf->exit_worker_handler == NULL
+        || lmcf->lua == NULL) 
+    {
+            return;


there should be 8 spaces.

membphis · 2017-01-03T03:38:28Z

t/151-exit-worker.t

@@ -0,0 +1,60 @@
+# vim:set ft= ts=4 sw=4 et fdm=marker:
+
+use Test::Nginx::Socket 'no_plan';


use plan tests is more better, its the default way.

sorry, i frist use test-nginx，so something not good.

membphis · 2017-01-03T03:40:27Z

t/151-exit-worker.t

+=== TEST 1: simple exit_worker_by_lua_block
+--- http_config
+    exit_worker_by_lua_block {
+        ngx.log(ngx.NOTICE, "log from exit_worker_by_lua_block")


we need to confirm if the log is output

i fix 151-exit-worker.t base on iresty/test-nginx and commited.

@lixianliang Apparently iresty/test-nginx is a fork and does not count here :) That feature needs to be merged into the official test-nginx repo before this can be merged otherwise the Travis CI would be failing.

Yes, Now i use offical test-nginx.

membphis · 2017-01-03T03:42:23Z

t/151-exit-worker.t

+--- http_config
+    exit_worker_by_lua_block {
+        ngx.log(ngx.NOTICE, "log from exit_worker_by_lua_block")
+        -- grep_error_log chop


We'd better delete useless lines from 18-23

I met a problem when write exit_worker test。this log print when worker process eixtting

I have submitted a PR openresty/test-nginx#52

You can make the test case with this version.

Here is the example:

=== TEST 1: --- http_config --- config location /t { echo "ok"; } --- request GET /t --- stop_after_request --- error_log received, shutting down

i make test base on iresty/test-nginx and resubmit 151-exit-worker.t

lixianliang · 2017-01-03T10:35:14Z

fix code style

agentzh · 2017-01-06T18:43:00Z

README.markdown

+
+[Back to TOC](#directives)
+
+exit_worker_by_lua_block


Typo? Should be _by_lua_file here instead, right?

My mistake.

agentzh · 2017-01-06T18:43:59Z

README.markdown

@@ -1013,6 +1013,8 @@ Directives
 * [init_worker_by_lua](#init_worker_by_lua)


BTW, this file is automatically generated (you can check out the comment at the beginning of this file).

You should edit doc/HttpLuaModule.wiki file instead and use the ngx-releng tool chain to update this markdown file automatically from the .wiki file.

agentzh · 2017-01-06T18:45:06Z

t/151-exit-worker.t

+--- response_body
+ok
+--- stop_after_request
+--- error_log


We need to add a --- shutdown_error_log section instead of abusing the request-time --- error_log directive and the hacky --- stop_after_request section IMHO :)

This test cast i update util has shutdown_error_log.

add NGX_HTTP_LUA_CONTEXT_EXIT_WORKER and tests

lixianliang · 2017-02-08T08:59:12Z

@membphis can you review this exit_worker feature?

agentzh · 2017-02-08T20:05:16Z

doc/HttpLuaModule.wiki

+
+Runs the specified Lua code upon every Nginx worker process's exiting when the master process is enabled.
+
+This hook is often used to release resources when init_worker_by* create resources, To prevent the worker exit abnormal.


init_worker_by* => init_worker_by_lua*.

Also need <code></code> around init_worker_by_lua*.

Also, better add a link to the init_worker_by_lua* label.

@agentzh I add init_worker_by_lua* label to init_worker_by_lua_block not init_worker_by_lua. I want recommend others use init_worker_by_lua_block. It has a problem?

agentzh · 2017-02-08T20:09:13Z

t/152-hup-master-on-exit-worker.t

+GET /t
+--- response_body
+ok
+--- shutdown_error_log


Do we have shutdown_error_log already? It seems that your test plan above explicitly excludes this check.

@membphis I think you were working on this feature for Test::Nginx?

@agentzh shutdown_error_log already use.

shutdown_error_log does not support the TEST_NGINX_USE_HUP flag.

TEST_NGINX_USE_HUP needs to keep nginx working, shutdown_error_log needs nginx shutdown.
They are in conflict.

membphis · 2017-02-09T15:42:33Z

@agentzh @lixianliang

I have submitted the PR: openresty/test-nginx#58

membphis · 2017-02-10T05:42:51Z

t/151-exit-worker.t

+--- response_body
+ok
+--- shutdown_error_log
+get 11val: 1000


it seems to wrong

OK，I fix, but use --- shutdown_error_log test success，so shutdown_error_log has bug?

agentzh · 2017-02-11T21:36:51Z

t/151-exit-worker.t

+ok
+--- shutdown_error_log
+get val: 100
+


Better remove this blank line.

agentzh · 2017-02-12T00:10:26Z

t/151-exit-worker.t

+
+repeat_each(2);
+
+plan tests => repeat_each() * (blocks() * 2);


Because the official test-nginx does not come with shutdown_error_log yet and your branch is currently passing the travis ci tests completely, which means this test plan is wrong since it does not count the shutdown_error_log test in the first place.

agentzh · 2017-03-03T20:12:13Z

Please rebase to the latest master. Thank you!

lixianliang · 2017-03-05T12:56:27Z

@agentzh fix

agentzh · 2017-03-07T18:32:10Z

@lixianliang Great! Thank you! We'll look into this shortly.

lixianliang · 2017-04-19T06:23:16Z

@agentzh
I hope this feature to merge early， Thranks.

agentzh · 2017-04-19T20:08:39Z

@lixianliang Will take care of this as soon as I can manage. Sorry for the delay on my side!

lixianliang · 2017-05-06T01:06:01Z

@agentzh ok，Thanks

rainingmaster · 2020-03-27T02:09:35Z

@agentzh @doujiang24 it seem other guy need this feature too, openresty/openresty#597, will we merge it into master in future?

doujiang24

@lixianliang Sorry for the long delay, will you please rebase to the latest master? there are some conflicts, thanks :)

doujiang24 · 2020-03-29T13:40:23Z

src/ngx_http_lua_directive.c

+    ngx_str_t                   *value;
+    ngx_http_lua_main_conf_t    *lmcf = conf;
+
+    dd("enter");


better use more meaning use debug message here. people will see this message in the error log when they opened the DDEBUG.

mergify · 2020-06-26T00:41:14Z

This pull request is now in conflict :(

doujiang24 · 2020-07-23T03:37:02Z

Thanks, already merged in #1682.

add exit_worker_by* feature

712fd57

thibaultcha reviewed Dec 13, 2016

View reviewed changes

agentzh requested changes Dec 13, 2016

View reviewed changes

letian added 2 commits January 2, 2017 08:30

add exit feature

5d24fa1

Merge remote-tracking branch 'upstream/master'

d08f950

membphis reviewed Jan 2, 2017

View reviewed changes

letian added 2 commits January 3, 2017 10:31

add exit-worker test and exit_worker_by* the manual

cc40d62

When => when

ab3e195

membphis reviewed Jan 3, 2017

View reviewed changes

fix code style

0dac2a1

fix 151-exit-worker.t base on iresty/test-nginx

b3b3abc

agentzh reviewed Jan 6, 2017

View reviewed changes

letian and others added 4 commits January 7, 2017 11:27

upgrade markdown and wiki

262bb28

fix exit_worker_by_lua_file HttpLuaModule.wiki

59bfe2b

add NGX_HTTP_LUA_CONTEXT_EXIT_WORKER and tests

754e555

Merge pull request #1 from rainingmaster/exitworker

affda8a

add NGX_HTTP_LUA_CONTEXT_EXIT_WORKER and tests

agentzh requested changes Feb 8, 2017

View reviewed changes

agentzh reviewed Feb 8, 2017

View reviewed changes

fix wiki

11a0359

membphis reviewed Feb 10, 2017

View reviewed changes

fix 151-exit-worker.t test

0657a39

agentzh reviewed Feb 11, 2017

View reviewed changes

remove 151-exit-worker.t blank line

a598365

agentzh reviewed Feb 12, 2017

View reviewed changes

rebase laster master

2141d4b

thibaultcha mentioned this pull request Mar 10, 2017

about exit_by_lua #1010

Closed

Merge remote-tracking branch 'upstream/master'

d53ad01

Merge remote-tracking branch 'upstream/master'

b9474bf

thibaultcha force-pushed the master branch from a79e9aa to 315330a Compare August 27, 2019 21:35

rainingmaster mentioned this pull request Mar 26, 2020

add directives to master exit, worker exit and config reload openresty/openresty#597

Open

doujiang24 reviewed Mar 29, 2020

View reviewed changes

rainingmaster mentioned this pull request Apr 3, 2020

add exit_worker_by* feature #1682

Merged

mergify bot added the conflict label Jun 26, 2020

doujiang24 closed this Jul 23, 2020

		@@ -0,0 +1,60 @@
		# vim:set ft= ts=4 sw=4 et fdm=marker:

		use Test::Nginx::Socket 'no_plan';

		@@ -1013,6 +1013,8 @@ Directives
		* [init_worker_by_lua](#init_worker_by_lua)


		Runs the specified Lua code upon every Nginx worker process's exiting when the master process is enabled.

		This hook is often used to release resources when init_worker_by* create resources, To prevent the worker exit abnormal.


		repeat_each(2);

		plan tests => repeat_each() * (blocks() * 2);

add exit_worker_by* feature #927

add exit_worker_by* feature #927

Uh oh!

Conversation

lixianliang commented Dec 13, 2016

Uh oh!

agentzh commented Dec 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thibaultcha Dec 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lixianliang commented Dec 13, 2016

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agentzh commented Dec 13, 2016

Uh oh!

agentzh commented Dec 13, 2016

Uh oh!

thibaultcha commented Dec 13, 2016

Uh oh!

thibaultcha commented Dec 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lixianliang commented Jan 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

membphis Jan 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lixianliang commented Jan 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thibaultcha Dec 13, 2016 •

edited

Loading

thibaultcha commented Dec 13, 2016 •

edited

Loading

membphis Jan 5, 2017 •

edited

Loading

membphis Feb 10, 2017 •

edited

Loading