fix: worker not exited when executing quit or reload command

[jiangfucheng]

Description

Fixes #9802

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 first)

[monkeyDluffy6017]

How long does the long connection prevent the quit operation?

[kingluo]

@jiangfucheng Please show your steps to reproduce this issue.

[jiangfucheng]

How long does the long connection prevent the quit operation?

Accoding my test, the worker will never exit.

[jiangfucheng]

@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

[kingluo]

@jiangfucheng
The code is ok.
Please check the failed ci: t/node/healthcheck-stop-checker.t.

[jiangfucheng]

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 #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.

[kingluo]

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!

[monkeyDluffy6017]

What if the thread do_run_watch crashes ?

[kingluo]

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.

[kingluo]

Frankly, restarting it in this scenario is an over-design.

[monkeyDluffy6017]

Agreed with @kingluo, restart is unnecessary

[jiangfucheng]

Updated, thank you for your review.❤️

[kingluo]

@jiangfucheng The code is ok. I will approve it after the ci finishes successfully.

[jiangfucheng]

@monkeyDluffy6017 @kingluo Please take a look again, thanks.❤️

[moonming]

@kingluo please take a look again