Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cli): shut down timers at the end of execution #9536

Merged
merged 3 commits into from
Oct 17, 2022
Merged

Conversation

flrgh
Copy link
Contributor

@flrgh flrgh commented Oct 11, 2022

Summary

With timer-ng, the CLI has become slow to terminate as nginx attempts to wait for all pending timers to stop before exiting:

$ time kong roar >/dev/null

real    0m1.033s
user    0m0.024s
sys     0m0.016s

This commit explicitly shuts down the timer-ng instance so that the CLI can quickly exit:

$ time kong roar >/dev/null

real    0m0.132s
user    0m0.027s
sys     0m0.013s

Full changelog

  • explicitly stop timer-ng instance on completion of CLI commands

@flrgh flrgh requested a review from a team as a code owner October 11, 2022 17:46
@bungle bungle requested a review from ADD-SP October 11, 2022 17:52
@bungle
Copy link
Member

bungle commented Oct 11, 2022

@ADD-SP can you confirm this is alright?

kong/cmd/init.lua Outdated Show resolved Hide resolved
@Tieske
Copy link
Member

Tieske commented Oct 13, 2022

wondering; shouldn't this be fixed in timer-ng instead?

@flrgh
Copy link
Contributor Author

flrgh commented Oct 13, 2022

wondering; shouldn't this be fixed in timer-ng instead?

I think timer-ng is mostly behaving as expected here. I believe the 1s shutdown delay observed currently is due to a semaphore:wait(1) call somewhere (as controlled by this constant). Delaying worker shutdown by as much as 1s seems totally fine in the regular nginx context--it's just that the CLI is a whole different beast.

@dndx dndx merged commit 26b2db7 into master Oct 17, 2022
@dndx dndx deleted the fix/cli-stop-timers branch October 17, 2022 03:59
@Tieske
Copy link
Member

Tieske commented Oct 17, 2022

wondering; shouldn't this be fixed in timer-ng instead?

I think timer-ng is mostly behaving as expected here. I believe the 1s shutdown delay observed currently is due to a semaphore:wait(1) call somewhere (as controlled by this constant). Delaying worker shutdown by as much as 1s seems totally fine in the regular nginx context--it's just that the CLI is a whole different beast.

this is an old problem caused by a lack of IO in the nginx control loop, workarounds are available. See openresty/lua-resty-core#337 That said; from that issue I see that I already pointed that out before...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants