Repository navigation
fix(http): classic shutdown waits for every handler before the app goes - #362
Merged
Merged
Conversation
Classic pool handlers run on detached workers. A keep-alive connection keeps its worker in a blocking read for as long as the client holds it open, and a client that keeps sending requests keeps it dispatching. Shutdown only waited for the in-flight count up to the drain timeout and then destroyed the app, so cwist_app_destroy() freed the route table under a worker still in cwist_route_table_lookup (the open TSan report in test_http_client, #351 follow-up). Each pool worker now registers the connection it serves, and cwist_http_pool_destroy() drains them before returning: 1. idle keep-alive connections are shut down at once; 2. in-flight requests get the drain timeout to finish; 3. what is left is shut down (SHUT_RDWR), and the pool waits until every handler has returned. A handler stuck in route code is waited for, with a warning every 5 s, rather than freed under. Handlers mark the wait for the next request with cwist_http_live_idle_enter()/_leave() (src/net/http/http_live.h): the header wait in cwist_http_receive_request() and the BDR fast path's recv in cwist_app_http_handler(). Once the pool is stopping, idle_enter returns false and the handler closes the connection instead of waiting. The handler serves a dup of the socket; the pool keeps the original open until the entry is unlinked, so shutdown() never reaches a reused descriptor. tests/test_classic_shutdown.c: an idle keep-alive connection no longer holds the drain (0.02 s vs 4.01 s before), an in-flight /slow request still gets its full response, and a client hammering its keep-alive connection cannot keep the server alive. test_http_client runs under TSan again.
WASI has no dup() or shutdown(), so wasm32-wasip2 failed to build (component pipeline). There the pool serves connections unregistered, as before, and the shutdown kick is a no-op.
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.
Follow-up to #351 (the open TSan report in
test_http_client).Bug
Classic pool handlers run on detached workers. A keep-alive connection keeps its worker in a blocking read for as long as the client holds it open (the BDR fast path's
recvhas no timeout at all), and a client that keeps sending requests keeps it dispatching. Shutdown waited for the in-flight count only up to the drain timeout, thencwist_app_destroy()freed the route table under a worker still incwist_route_table_lookup.Fix
Each pool worker registers the connection it serves;
cwist_http_pool_destroy()drains them before it returns (and before the caller destroys the app):CWIST_DRAIN_TIMEOUT) to finish;SHUT_RDWR) and the pool waits until every handler has returned. A handler stuck in route code is waited for (warning every 5 s) rather than freed under, the same contract C1M already has (it joins its handler threads).cwist_http_live_idle_enter()/_leave()(privatesrc/net/http/http_live.h): the header wait incwist_http_receive_request()and the BDRrecvincwist_app_http_handler(). Once the pool is stopping,idle_enterreturns false and the handler closes instead of waiting. Outside a classic pool worker both are no-ops.dupof the socket; the pool keeps the original open until the registry entry is unlinked, so ashutdown()issued under the registry lock never reaches a reused descriptor. Cost: onedup+closeper classic connection (not per request), and one extra descriptor while it is served.Scope: the plain-HTTP classic pool. The HTTPS pool and the forking path are unchanged.
Tests
tests/test_classic_shutdown.c(classic, one worker, 4 s drain):dev);/slow(1 s handler) still gets its complete response;test_http_clientremoved fromTSAN_EXCLUDE.test_http_client,test_classic_shutdown,test_listen_ex,test_classic_pool_scaling,test_shutdown— all pass. Normal build: the same plustest_static_and_range,test_async_defer,test_https_metricspass.malloc_lint.sh,check_api_baseline.py,check_test_wiring.pyOK (no public API change).