You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "jiangfucheng (via GitHub)" <gi...@apache.org> on 2023/07/26 13:15:35 UTC
[GitHub] [apisix] jiangfucheng opened a new pull request, #9909: fix: worker not exited when executing quit or reload command
jiangfucheng opened a new pull request, #9909:
URL: https://github.com/apache/apisix/pull/9909
### Description
<!-- Please include a summary of the change and which issue is fixed. -->
<!-- Please also include relevant motivation and context. -->
Fixes # (issue)
### Checklist
- [ ] I have explained the need for this PR and the problem it solves
- [ ] I have explained the changes or the new features added to this PR
- [ ] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [ ] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kingluo commented on a diff in pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "kingluo (via GitHub)" <gi...@apache.org>.
kingluo commented on code in PR #9909:
URL: https://github.com/apache/apisix/pull/9909#discussion_r1275675442
##########
apisix/core/config_etcd.lua:
##########
@@ -188,9 +190,43 @@ local function run_watch(premature)
::watch_event::
while true do
- local res, err = res_func()
- if log_level >= NGX_INFO then
- log.info("res_func: ", inspect(res))
+ local res
+ local sema, err = semaphore.new()
+ if not sema then
+ log.error("create sema failed", err)
+ goto watch_event
+ end
+
+ local get_res_th = ngx_thread_spawn(function ()
Review Comment:
@leslie-tsang
I'm concerned that each watch will spawn two threads, which will affect performance.
What if we check exiting() after the watch has timed out? It's currently 50 seconds, maybe we should lower it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kingluo commented on a diff in pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "kingluo (via GitHub)" <gi...@apache.org>.
kingluo commented on code in PR #9909:
URL: https://github.com/apache/apisix/pull/9909#discussion_r1282687319
##########
apisix/core/config_etcd.lua:
##########
@@ -188,9 +190,43 @@ local function run_watch(premature)
::watch_event::
while true do
- local res, err = res_func()
- if log_level >= NGX_INFO then
- log.info("res_func: ", inspect(res))
+ local res
+ local sema, err = semaphore.new()
+ if not sema then
+ log.error("create sema failed", err)
+ goto watch_event
+ end
+
+ local get_res_th = ngx_thread_spawn(function ()
Review Comment:
Forget that, it doesn't seem like a good point.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9909:
URL: https://github.com/apache/apisix/pull/9909#discussion_r1285616718
##########
apisix/core/config_etcd.lua:
##########
@@ -257,6 +260,33 @@ local function run_watch(premature)
end
+local function run_watch(premature)
+ local run_watch_th = ngx_thread_spawn(do_run_watch, premature)
+
+ ::restart::
+ local check_worker_th = ngx_thread_spawn(function ()
+ while not exiting() do
+ ngx_sleep(0.1)
+ end
+ end)
+
+ local ok, err = ngx_thread_wait(check_worker_th)
Review Comment:
I think you should monitor the `run_watch_th` too in case that the `run_watch_th` fails
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jiangfucheng commented on pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "jiangfucheng (via GitHub)" <gi...@apache.org>.
jiangfucheng commented on PR #9909:
URL: https://github.com/apache/apisix/pull/9909#issuecomment-1652803616
> How long does the long connection prevent the quit operation?
Accoding my test, the worker will never exit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9909:
URL: https://github.com/apache/apisix/pull/9909#discussion_r1285953148
##########
apisix/core/config_etcd.lua:
##########
@@ -257,6 +260,33 @@ local function run_watch(premature)
end
+local function run_watch(premature)
+ local run_watch_th = ngx_thread_spawn(do_run_watch, premature)
+
+ ::restart::
+ local check_worker_th = ngx_thread_spawn(function ()
+ while not exiting() do
+ ngx_sleep(0.1)
+ end
+ end)
+
+ local ok, err = ngx_thread_wait(check_worker_th)
Review Comment:
There is nothing to do with the normal return from the `do_run_watch` function.
the first value returned by `ngx.thread.wait` is a boolean value indicating whether the "light thread" terminates successfully or not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9909:
URL: https://github.com/apache/apisix/pull/9909#discussion_r1293171340
##########
apisix/core/config_etcd.lua:
##########
@@ -257,6 +260,30 @@ local function run_watch(premature)
end
+local function run_watch(premature)
+ ::restart::
Review Comment:
What if the thread `do_run_watch` crashes ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9909:
URL: https://github.com/apache/apisix/pull/9909#discussion_r1293294374
##########
apisix/core/config_etcd.lua:
##########
@@ -257,6 +260,30 @@ local function run_watch(premature)
end
+local function run_watch(premature)
+ ::restart::
Review Comment:
Agreed with @kingluo, restart is unnecessary
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9909:
URL: https://github.com/apache/apisix/pull/9909#discussion_r1285961493
##########
apisix/core/config_etcd.lua:
##########
@@ -257,6 +260,33 @@ local function run_watch(premature)
end
+local function run_watch(premature)
+ local run_watch_th = ngx_thread_spawn(do_run_watch, premature)
+
+ ::restart::
+ local check_worker_th = ngx_thread_spawn(function ()
+ while not exiting() do
+ ngx_sleep(0.1)
+ end
+ end)
+
+ local ok, err = ngx_thread_wait(check_worker_th)
Review Comment:
We should handle the situation that the `run_watch_th` thread crashes
##########
apisix/core/config_etcd.lua:
##########
@@ -257,6 +260,33 @@ local function run_watch(premature)
end
+local function run_watch(premature)
+ local run_watch_th = ngx_thread_spawn(do_run_watch, premature)
+
+ ::restart::
+ local check_worker_th = ngx_thread_spawn(function ()
+ while not exiting() do
+ ngx_sleep(0.1)
+ end
+ end)
+
+ local ok, err = ngx_thread_wait(check_worker_th)
Review Comment:
We should handle the case that the `run_watch_th` thread crashes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kingluo commented on a diff in pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "kingluo (via GitHub)" <gi...@apache.org>.
kingluo commented on code in PR #9909:
URL: https://github.com/apache/apisix/pull/9909#discussion_r1293163630
##########
apisix/core/config_etcd.lua:
##########
@@ -257,6 +260,30 @@ local function run_watch(premature)
end
+local function run_watch(premature)
+ ::restart::
Review Comment:
**No! The watch routine must not be restarted!**
Because it's a stateful routine (e.g. last watch starts revision) and should start once and notify all child watchers that it started.
@jiangfucheng
**Instead, you just wait for check_worker_th, if it exits, it means the worker process is exiting, then kill run_watch_th, and that's it.**
Please make a change again on the code, thank you very much!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kingluo commented on pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "kingluo (via GitHub)" <gi...@apache.org>.
kingluo commented on PR #9909:
URL: https://github.com/apache/apisix/pull/9909#issuecomment-1677431084
@jiangfucheng The code is ok. I will approve it after the ci finishes successfully.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kingluo commented on a diff in pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "kingluo (via GitHub)" <gi...@apache.org>.
kingluo commented on code in PR #9909:
URL: https://github.com/apache/apisix/pull/9909#discussion_r1282685843
##########
apisix/core/config_etcd.lua:
##########
@@ -188,9 +191,36 @@ local function run_watch(premature)
::watch_event::
while true do
- local res, err = res_func()
- if log_level >= NGX_INFO then
- log.info("res_func: ", inspect(res))
+ local res
+
+ local get_res_th = ngx_thread_spawn(function ()
Review Comment:
The code is fine, but there is a slight impact on performance, because each res_func() will spawn a new thread, and this thread will exit after 50 seconds.
So, is it feasible to run the whole run_watch() in one thread and watch for the exit signal in another thread?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9909:
URL: https://github.com/apache/apisix/pull/9909#discussion_r1286076702
##########
apisix/core/config_etcd.lua:
##########
@@ -257,6 +260,33 @@ local function run_watch(premature)
end
+local function run_watch(premature)
+ local run_watch_th = ngx_thread_spawn(do_run_watch, premature)
+
+ ::restart::
+ local check_worker_th = ngx_thread_spawn(function ()
+ while not exiting() do
+ ngx_sleep(0.1)
+ end
+ end)
+
+ local ok, err = ngx_thread_wait(check_worker_th)
Review Comment:
I think the code may like this
```
::restart::
local run_watch_th = ngx_thread_spawn(do_run_watch, premature)
local check_worker_th = ngx_thread_spawn(function ()
while not exiting() do
ngx_sleep(0.1)
end
end)
local ok, err = ngx_thread_wait(run_watch_th, check_worker_th)
if not ok then
log.error("wait worker checker failed, retart checker, error: " .. err)
ngx_thread_kill(run_watch_th)
ngx_thread_kill(check_worker_th)
goto restart
end
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jiangfucheng commented on a diff in pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "jiangfucheng (via GitHub)" <gi...@apache.org>.
jiangfucheng commented on code in PR #9909:
URL: https://github.com/apache/apisix/pull/9909#discussion_r1287072160
##########
apisix/core/config_etcd.lua:
##########
@@ -257,6 +260,33 @@ local function run_watch(premature)
end
+local function run_watch(premature)
+ local run_watch_th = ngx_thread_spawn(do_run_watch, premature)
+
+ ::restart::
+ local check_worker_th = ngx_thread_spawn(function ()
+ while not exiting() do
+ ngx_sleep(0.1)
+ end
+ end)
+
+ local ok, err = ngx_thread_wait(check_worker_th)
+
+ if not ok then
+ log.error("wait worker checker failed, retart checker, error: " .. err)
+ ngx_thread_kill(check_worker_th)
+ goto restart
+ end
+
+ -- waiting for do_run_watch thread to handle the final etcd event
+ ngx_sleep(1)
Review Comment:
removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jiangfucheng commented on pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "jiangfucheng (via GitHub)" <gi...@apache.org>.
jiangfucheng commented on PR #9909:
URL: https://github.com/apache/apisix/pull/9909#issuecomment-1652804626
> @jiangfucheng Please show your steps to reproduce this issue.
1. make run
2. make reload or make quit
3. ` ps -ef |grep nginx` check the worker if exited
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9909:
URL: https://github.com/apache/apisix/pull/9909#discussion_r1285957481
##########
apisix/core/config_etcd.lua:
##########
@@ -257,6 +260,33 @@ local function run_watch(premature)
end
+local function run_watch(premature)
+ local run_watch_th = ngx_thread_spawn(do_run_watch, premature)
+
+ ::restart::
+ local check_worker_th = ngx_thread_spawn(function ()
+ while not exiting() do
+ ngx_sleep(0.1)
+ end
+ end)
+
+ local ok, err = ngx_thread_wait(check_worker_th)
+
+ if not ok then
+ log.error("wait worker checker failed, retart checker, error: " .. err)
+ ngx_thread_kill(check_worker_th)
+ goto restart
+ end
+
+ -- waiting for do_run_watch thread to handle the final etcd event
+ ngx_sleep(1)
Review Comment:
Can we fix the test cases instead
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jiangfucheng commented on a diff in pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "jiangfucheng (via GitHub)" <gi...@apache.org>.
jiangfucheng commented on code in PR #9909:
URL: https://github.com/apache/apisix/pull/9909#discussion_r1285906693
##########
apisix/core/config_etcd.lua:
##########
@@ -257,6 +260,33 @@ local function run_watch(premature)
end
+local function run_watch(premature)
+ local run_watch_th = ngx_thread_spawn(do_run_watch, premature)
+
+ ::restart::
+ local check_worker_th = ngx_thread_spawn(function ()
+ while not exiting() do
+ ngx_sleep(0.1)
+ end
+ end)
+
+ local ok, err = ngx_thread_wait(check_worker_th)
Review Comment:
The `do_run_watch` function will not return, it has an internal retry mechanism. The origin logic did not handle this so I didn't do this either.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jiangfucheng commented on a diff in pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "jiangfucheng (via GitHub)" <gi...@apache.org>.
jiangfucheng commented on code in PR #9909:
URL: https://github.com/apache/apisix/pull/9909#discussion_r1293502797
##########
apisix/core/config_etcd.lua:
##########
@@ -257,6 +260,30 @@ local function run_watch(premature)
end
+local function run_watch(premature)
+ ::restart::
Review Comment:
Updated, thank you for your review.❤️
##########
apisix/core/config_etcd.lua:
##########
@@ -257,6 +260,30 @@ local function run_watch(premature)
end
+local function run_watch(premature)
+ ::restart::
Review Comment:
Updated, thank you for your review.❤️
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jiangfucheng commented on a diff in pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "jiangfucheng (via GitHub)" <gi...@apache.org>.
jiangfucheng commented on code in PR #9909:
URL: https://github.com/apache/apisix/pull/9909#discussion_r1285954238
##########
apisix/core/config_etcd.lua:
##########
@@ -257,6 +260,33 @@ local function run_watch(premature)
end
+local function run_watch(premature)
+ local run_watch_th = ngx_thread_spawn(do_run_watch, premature)
+
+ ::restart::
+ local check_worker_th = ngx_thread_spawn(function ()
+ while not exiting() do
+ ngx_sleep(0.1)
+ end
+ end)
+
+ local ok, err = ngx_thread_wait(check_worker_th)
+
+ if not ok then
+ log.error("wait worker checker failed, retart checker, error: " .. err)
+ ngx_thread_kill(check_worker_th)
+ goto restart
+ end
+
+ -- waiting for do_run_watch thread to handle the final etcd event
+ ngx_sleep(1)
Review Comment:
Yes, it is not necessary. `t/node/healthcheck-stop-checker.t.` this test case failed seems related to this, so I add the logic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9909:
URL: https://github.com/apache/apisix/pull/9909#discussion_r1285948465
##########
apisix/core/config_etcd.lua:
##########
@@ -257,6 +260,33 @@ local function run_watch(premature)
end
+local function run_watch(premature)
+ local run_watch_th = ngx_thread_spawn(do_run_watch, premature)
+
+ ::restart::
+ local check_worker_th = ngx_thread_spawn(function ()
+ while not exiting() do
+ ngx_sleep(0.1)
+ end
+ end)
+
+ local ok, err = ngx_thread_wait(check_worker_th)
+
+ if not ok then
+ log.error("wait worker checker failed, retart checker, error: " .. err)
+ ngx_thread_kill(check_worker_th)
+ goto restart
+ end
+
+ -- waiting for do_run_watch thread to handle the final etcd event
+ ngx_sleep(1)
Review Comment:
By the way, do we really need to do this before quit? we will load full data after reload
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jiangfucheng commented on a diff in pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "jiangfucheng (via GitHub)" <gi...@apache.org>.
jiangfucheng commented on code in PR #9909:
URL: https://github.com/apache/apisix/pull/9909#discussion_r1287081976
##########
t/node/healthcheck-stop-checker.t:
##########
@@ -256,7 +247,6 @@ ok
--- grep_error_log eval
qr/create new checker: table: 0x|try to release checker: table: 0x/
--- grep_error_log_out
-try to release checker: table: 0x
Review Comment:
This line can be remove, the reasons are as follows:
1.`healthchekcer` will be remove after worker exit since the status store in memroy, that's why `TEST 5` will not print `try to release checker: table: 0x` in first line.
2.Why the `try to release checker: table: 0x` will be print if we add `sleep(1)` before worker exited
- Because the log is printed by old worker. Before old worker be killed, the new worker will be created, at this moment, both old worker and new worker can receive events, it's easy to be proved through print debug log before execute `fire_all_clean_handlers`
```
if pre_index then
local pre_val = self.values[pre_index]
log.info("sync_data: check pre_val: ", inspect(pre_val.clean_handlers), worker_id_str, " pid: ", ngx.worker.pid())
if pre_val then
config_util.fire_all_clean_handlers(pre_val)
end
```
logs:
```
2023/08/08 18:04:31 [info] 77582#1004031: *312 [lua] config_etcd.lua:741: sync_data(): sync_data: check pre_val: { {
f = <function 1>,
id = 1
},
_id = 2
} worker_id: 0 pid: 77582, context: ngx.timer
2023/08/08 18:04:31 [info] 77620#1004305: *447 [lua] config_etcd.lua:741: sync_data(): sync_data: check pre_val: {} worker_id: 0 pid: 77620, context: ngx.timer
```
We can see the etcd events be received with two `wokrer 0`, and there pid is different, we can easliy to confirm the worker is old worker which has `check_handlers` field
3.Why these test cases can passed in old version(before https://github.com/apache/apisix/issues/9456 be merged)
- Because in the old version, the worker is not exit immediately too, it will exit after `make quit/reload` about 60s, so the reason is same as above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kingluo commented on a diff in pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "kingluo (via GitHub)" <gi...@apache.org>.
kingluo commented on code in PR #9909:
URL: https://github.com/apache/apisix/pull/9909#discussion_r1293182074
##########
apisix/core/config_etcd.lua:
##########
@@ -257,6 +260,30 @@ local function run_watch(premature)
end
+local function run_watch(premature)
+ ::restart::
Review Comment:
> What if the thread `do_run_watch` crashes?
It's not supposed to crash.
In fact, almost all timers did not have a crash guard.
And even if run_watch crashes, you cannot fix it by restarting it, 'cause it's a stateful routine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jiangfucheng commented on a diff in pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "jiangfucheng (via GitHub)" <gi...@apache.org>.
jiangfucheng commented on code in PR #9909:
URL: https://github.com/apache/apisix/pull/9909#discussion_r1286028107
##########
apisix/core/config_etcd.lua:
##########
@@ -257,6 +260,33 @@ local function run_watch(premature)
end
+local function run_watch(premature)
+ local run_watch_th = ngx_thread_spawn(do_run_watch, premature)
+
+ ::restart::
+ local check_worker_th = ngx_thread_spawn(function ()
+ while not exiting() do
+ ngx_sleep(0.1)
+ end
+ end)
+
+ local ok, err = ngx_thread_wait(check_worker_th)
Review Comment:
Do we need restart `run_watch_th` when it fails?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming merged pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "moonming (via GitHub)" <gi...@apache.org>.
moonming merged PR #9909:
URL: https://github.com/apache/apisix/pull/9909
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jiangfucheng commented on pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "jiangfucheng (via GitHub)" <gi...@apache.org>.
jiangfucheng commented on PR #9909:
URL: https://github.com/apache/apisix/pull/9909#issuecomment-1680701007
@monkeyDluffy6017 @kingluo Please take a look again, thanks.❤️
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on PR #9909:
URL: https://github.com/apache/apisix/pull/9909#issuecomment-1652797295
How long does the long connection prevent the quit operation?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jiangfucheng commented on a diff in pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "jiangfucheng (via GitHub)" <gi...@apache.org>.
jiangfucheng commented on code in PR #9909:
URL: https://github.com/apache/apisix/pull/9909#discussion_r1282939189
##########
apisix/core/config_etcd.lua:
##########
@@ -188,9 +191,36 @@ local function run_watch(premature)
::watch_event::
while true do
- local res, err = res_func()
- if log_level >= NGX_INFO then
- log.info("res_func: ", inspect(res))
+ local res
+
+ local get_res_th = ngx_thread_spawn(function ()
Review Comment:
Good idea. I have a question, can we use `ngx_thread_spawn` instead of `ngx_timer_at`? we need kill the `run_watch` thread when worker exited.
https://github.com/apache/apisix/blob/c89ec15e85eded367bdc5e03fa61ac5f6e53792d/apisix/core/config_etcd.lua#L269
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9909:
URL: https://github.com/apache/apisix/pull/9909#discussion_r1285615841
##########
apisix/core/config_etcd.lua:
##########
@@ -257,6 +260,33 @@ local function run_watch(premature)
end
+local function run_watch(premature)
+ local run_watch_th = ngx_thread_spawn(do_run_watch, premature)
+
+ ::restart::
+ local check_worker_th = ngx_thread_spawn(function ()
+ while not exiting() do
+ ngx_sleep(0.1)
+ end
+ end)
+
+ local ok, err = ngx_thread_wait(check_worker_th)
+
+ if not ok then
+ log.error("wait worker checker failed, retart checker, error: " .. err)
+ ngx_thread_kill(check_worker_th)
+ goto restart
+ end
+
+ -- waiting for do_run_watch thread to handle the final etcd event
+ ngx_sleep(1)
Review Comment:
How do you know no more etcd events will come in the last 1 second?
I think you should check ngx.exiting in `do_run_watch` too
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9909:
URL: https://github.com/apache/apisix/pull/9909#discussion_r1282599477
##########
apisix/core/config_etcd.lua:
##########
@@ -188,9 +190,43 @@ local function run_watch(premature)
::watch_event::
while true do
- local res, err = res_func()
- if log_level >= NGX_INFO then
- log.info("res_func: ", inspect(res))
+ local res
+ local sema, err = semaphore.new()
+ if not sema then
+ log.error("create sema failed", err)
+ goto watch_event
+ end
+
+ local get_res_th = ngx_thread_spawn(function ()
Review Comment:
How long do you think is appropriate? 1s?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9909:
URL: https://github.com/apache/apisix/pull/9909#discussion_r1285934707
##########
apisix/core/config_etcd.lua:
##########
@@ -257,6 +260,33 @@ local function run_watch(premature)
end
+local function run_watch(premature)
+ local run_watch_th = ngx_thread_spawn(do_run_watch, premature)
+
+ ::restart::
+ local check_worker_th = ngx_thread_spawn(function ()
+ while not exiting() do
+ ngx_sleep(0.1)
+ end
+ end)
+
+ local ok, err = ngx_thread_wait(check_worker_th)
+
+ if not ok then
+ log.error("wait worker checker failed, retart checker, error: " .. err)
+ ngx_thread_kill(check_worker_th)
+ goto restart
+ end
+
+ -- waiting for do_run_watch thread to handle the final etcd event
+ ngx_sleep(1)
Review Comment:
The DP don't have admin API, it use etcd to sync the configuration from the CP which has admin API, when you execute `make quit` on DP, the CP is available.
So you should stop syncing after you execute `make quit` on DP
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jiangfucheng commented on a diff in pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "jiangfucheng (via GitHub)" <gi...@apache.org>.
jiangfucheng commented on code in PR #9909:
URL: https://github.com/apache/apisix/pull/9909#discussion_r1286021445
##########
apisix/core/config_etcd.lua:
##########
@@ -257,6 +260,33 @@ local function run_watch(premature)
end
+local function run_watch(premature)
+ local run_watch_th = ngx_thread_spawn(do_run_watch, premature)
+
+ ::restart::
+ local check_worker_th = ngx_thread_spawn(function ()
+ while not exiting() do
+ ngx_sleep(0.1)
+ end
+ end)
+
+ local ok, err = ngx_thread_wait(check_worker_th)
+
+ if not ok then
+ log.error("wait worker checker failed, retart checker, error: " .. err)
+ ngx_thread_kill(check_worker_th)
+ goto restart
+ end
+
+ -- waiting for do_run_watch thread to handle the final etcd event
+ ngx_sleep(1)
Review Comment:
I will try it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kingluo commented on pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "kingluo (via GitHub)" <gi...@apache.org>.
kingluo commented on PR #9909:
URL: https://github.com/apache/apisix/pull/9909#issuecomment-1652801638
@jiangfucheng Please show your steps to reproduce this issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jiangfucheng commented on a diff in pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "jiangfucheng (via GitHub)" <gi...@apache.org>.
jiangfucheng commented on code in PR #9909:
URL: https://github.com/apache/apisix/pull/9909#discussion_r1285900637
##########
apisix/core/config_etcd.lua:
##########
@@ -257,6 +260,33 @@ local function run_watch(premature)
end
+local function run_watch(premature)
+ local run_watch_th = ngx_thread_spawn(do_run_watch, premature)
+
+ ::restart::
+ local check_worker_th = ngx_thread_spawn(function ()
+ while not exiting() do
+ ngx_sleep(0.1)
+ end
+ end)
+
+ local ok, err = ngx_thread_wait(check_worker_th)
+
+ if not ok then
+ log.error("wait worker checker failed, retart checker, error: " .. err)
+ ngx_thread_kill(check_worker_th)
+ goto restart
+ end
+
+ -- waiting for do_run_watch thread to handle the final etcd event
+ ngx_sleep(1)
Review Comment:
After we execute `make quit` command, the admin API will not be available, so before the admin API becomes unavailable, we can use`sleep(1)` to wait for the `do_run_watch` thread to handle the last possible event. Even if no events come, we also will kill the `do_run_watch` thread.
And, check `ngx.exiting` in `do_run_watch` it means we need to wait for the `res_func()` function timeout, that's why I create two threads to handle it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jiangfucheng commented on a diff in pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "jiangfucheng (via GitHub)" <gi...@apache.org>.
jiangfucheng commented on code in PR #9909:
URL: https://github.com/apache/apisix/pull/9909#discussion_r1287073242
##########
apisix/core/config_etcd.lua:
##########
@@ -257,6 +260,33 @@ local function run_watch(premature)
end
+local function run_watch(premature)
+ local run_watch_th = ngx_thread_spawn(do_run_watch, premature)
+
+ ::restart::
+ local check_worker_th = ngx_thread_spawn(function ()
+ while not exiting() do
+ ngx_sleep(0.1)
+ end
+ end)
+
+ local ok, err = ngx_thread_wait(check_worker_th)
Review Comment:
I think it ok and updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jiangfucheng commented on a diff in pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "jiangfucheng (via GitHub)" <gi...@apache.org>.
jiangfucheng commented on code in PR #9909:
URL: https://github.com/apache/apisix/pull/9909#discussion_r1282939189
##########
apisix/core/config_etcd.lua:
##########
@@ -188,9 +191,36 @@ local function run_watch(premature)
::watch_event::
while true do
- local res, err = res_func()
- if log_level >= NGX_INFO then
- log.info("res_func: ", inspect(res))
+ local res
+
+ local get_res_th = ngx_thread_spawn(function ()
Review Comment:
Good idea. I have a question, can we use `ngx_thread_spawn` instead of `ngx_timer_at`? we need kill the `run_watch` thread when worker exited.
https://github.com/apache/apisix/blob/c89ec15e85eded367bdc5e03fa61ac5f6e53792d/apisix/core/config_etcd.lua#L269
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kingluo commented on pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "kingluo (via GitHub)" <gi...@apache.org>.
kingluo commented on PR #9909:
URL: https://github.com/apache/apisix/pull/9909#issuecomment-1664053754
@jiangfucheng
The code is ok.
Please check the failed ci: t/node/healthcheck-stop-checker.t.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kingluo commented on a diff in pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "kingluo (via GitHub)" <gi...@apache.org>.
kingluo commented on code in PR #9909:
URL: https://github.com/apache/apisix/pull/9909#discussion_r1293183777
##########
apisix/core/config_etcd.lua:
##########
@@ -257,6 +260,30 @@ local function run_watch(premature)
end
+local function run_watch(premature)
+ ::restart::
Review Comment:
Frankly, restarting it in this scenario is an over-design.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9909:
URL: https://github.com/apache/apisix/pull/9909#discussion_r1293294374
##########
apisix/core/config_etcd.lua:
##########
@@ -257,6 +260,30 @@ local function run_watch(premature)
end
+local function run_watch(premature)
+ ::restart::
Review Comment:
Agreed with @kingluo
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kingluo commented on a diff in pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "kingluo (via GitHub)" <gi...@apache.org>.
kingluo commented on code in PR #9909:
URL: https://github.com/apache/apisix/pull/9909#discussion_r1293163630
##########
apisix/core/config_etcd.lua:
##########
@@ -257,6 +260,30 @@ local function run_watch(premature)
end
+local function run_watch(premature)
+ ::restart::
Review Comment:
**No! The watch routine must not be restarted!**
Because it's a stateful routine (e.g. last watch starts revision) and should start once and notify all child watchers that it started. Instead, you just wait for check_worker_th, if it exits, it means the worker process is exiting, then kill run_watch_th, and that's it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming commented on pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "moonming (via GitHub)" <gi...@apache.org>.
moonming commented on PR #9909:
URL: https://github.com/apache/apisix/pull/9909#issuecomment-1681521636
@kingluo please take a look again
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] jiangfucheng commented on a diff in pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "jiangfucheng (via GitHub)" <gi...@apache.org>.
jiangfucheng commented on code in PR #9909:
URL: https://github.com/apache/apisix/pull/9909#discussion_r1275677384
##########
apisix/core/config_etcd.lua:
##########
@@ -188,9 +190,43 @@ local function run_watch(premature)
::watch_event::
while true do
- local res, err = res_func()
- if log_level >= NGX_INFO then
- log.info("res_func: ", inspect(res))
+ local res
+ local sema, err = semaphore.new()
+ if not sema then
+ log.error("create sema failed", err)
+ goto watch_event
+ end
+
+ local get_res_th = ngx_thread_spawn(function ()
+ res, err = res_func()
+ if log_level >= NGX_INFO then
+ log.info("res_func: ", inspect(res))
+ end
+ sema:post()
+ end)
+
+ local check_worker_th = ngx_thread_spawn(function ()
+ while not exiting() do
+ ngx_sleep(0.1)
+ end
+ sema:post()
+ end)
+
+ local _, sem_err = sema:wait(opts.timeout + 10)
Review Comment:
Great suggestion, I didn't notice this method before, I will update it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] kingluo commented on a diff in pull request #9909: fix: worker not exited when executing quit or reload command
Posted by "kingluo (via GitHub)" <gi...@apache.org>.
kingluo commented on code in PR #9909:
URL: https://github.com/apache/apisix/pull/9909#discussion_r1275673613
##########
apisix/core/config_etcd.lua:
##########
@@ -188,9 +190,43 @@ local function run_watch(premature)
::watch_event::
while true do
- local res, err = res_func()
- if log_level >= NGX_INFO then
- log.info("res_func: ", inspect(res))
+ local res
+ local sema, err = semaphore.new()
+ if not sema then
+ log.error("create sema failed", err)
+ goto watch_event
+ end
+
+ local get_res_th = ngx_thread_spawn(function ()
+ res, err = res_func()
+ if log_level >= NGX_INFO then
+ log.info("res_func: ", inspect(res))
+ end
+ sema:post()
+ end)
+
+ local check_worker_th = ngx_thread_spawn(function ()
+ while not exiting() do
+ ngx_sleep(0.1)
+ end
+ sema:post()
+ end)
+
+ local _, sem_err = sema:wait(opts.timeout + 10)
Review Comment:
Could we use ngx.thread.wait instead of sema?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org