Add graceful shutdown to wasmtime serve, fix flaky CI#10394
Merged
alexcrichton merged 2 commits intobytecodealliance:mainfrom Mar 13, 2025
Merged
Add graceful shutdown to wasmtime serve, fix flaky CI#10394alexcrichton merged 2 commits intobytecodealliance:mainfrom
wasmtime serve, fix flaky CI#10394alexcrichton merged 2 commits intobytecodealliance:mainfrom
Conversation
This commit fixes the spurious CI failure found [here][fail]. The problem here is that the infrastructure for testing `wasmtime serve` was not properly waiting for all output in `finish`. There's some infrastructure in the tests for spawning a subprocess and managing it, but prior to this commit there was no way to shut down the server gracefully and thus read all pending output from the child tasks. The specific problem is that a specific error message was expected in the logs after a request had been processed, but the `finish` method wasn't reading the message. The reason for this is that `finish` had to resort to `kill -9` on the child process as there was no other means of shutting it down. This meant that the print, which happened after request completion, might be killed and never happen. The solution ended up in this commit is to (a) add a `--shutdown-addr` CLI flag to `wasmtime serve` and (b) beef up handling of graceful shutdown. Previously ctrl-c was used to exit the server but it didn't do anything but drop all in-progress work. Instead graceful shutdown is now handled by breaking out of the `accept` loop and then waiting for all child tasks to complete, meaning that no http requests once received are cancelled. In addition to ctrl-c the `--shutdown-addr` is used to listen for a TCP connection which is a second means of cancellation. The reason for this is that sending ctrl-c to a process is not nearly as trivial on Windows as it is on Unix, so I didn't want to deal with all the console bits necessary to get that aligned. [fail]: https://github.com/bytecodealliance/wasmtime/actions/runs/13833291117/job/38702374924?pr=10390
dicej
approved these changes
Mar 13, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This commit fixes the spurious CI failure found here. The problem here is that the infrastructure for testing
wasmtime servewas not properly waiting for all output infinish. There's some infrastructure in the tests for spawning a subprocess and managing it, but prior to this commit there was no way to shut down the server gracefully and thus read all pending output from the child tasks.The specific problem is that a specific error message was expected in the logs after a request had been processed, but the
finishmethod wasn't reading the message. The reason for this is thatfinishhad to resort tokill -9on the child process as there was no other means of shutting it down. This meant that the print, which happened after request completion, might be killed and never happen.The solution ended up in this commit is to (a) add a
--shutdown-addrCLI flag towasmtime serveand (b) beef up handling of graceful shutdown. Previously ctrl-c was used to exit the server but it didn't do anything but drop all in-progress work. Instead graceful shutdown is now handled by breaking out of theacceptloop and then waiting for all child tasks to complete, meaning that no http requests once received are cancelled. In addition to ctrl-c the--shutdown-addris used to listen for a TCP connection which is a second means of cancellation. The reason for this is that sending ctrl-c to a process is not nearly as trivial on Windows as it is on Unix, so I didn't want to deal with all the console bits necessary to get that aligned.