feat: make shutdown() immediate rather than queued - #198
Open
mdmzfzl wants to merge 3 commits into
Open
Conversation
Shutdown was delivered as a `Command::Shutdown` / `ServerCommand::Shutdown` on the same bounded mpsc that carries requests, so it was processed strictly FIFO behind whatever was already queued. Each queued request runs to completion, so the delay before the task ended was the *sum* of the pending response timeouts, not one of them. Worse, `shutdown()` was an `async fn` that awaited `send()` on a bounded channel, so when the queue was full -- exactly when a caller most wants out -- the shutdown request itself blocked waiting for a slot. Signal it out of band with a `CancellationToken` instead, selected against the task's inner loop. The losing future is dropped, which unwinds the task at whichever await point it is parked on: a socket write, a read awaiting a response, or a retry backoff sleep. `Promise::drop` already fails requests with `RequestError::Shutdown`, so both the abandoned transaction and everything left in the queue report the right error with no extra plumbing. Because the signal no longer needs backpressure, and cancelling an already cancelled or already dead task is a no-op rather than a failure, both methods become `pub fn shutdown(&self)` -- callable from a `Drop` impl, a signal handler, or any non-async context. Notes on the design: * The selects are `biased` with cancellation first. The inner loop is an infinite future, so an unbiased select would let it run whatever work was already ready before noticing the shutdown, roughly half the time. * The client's select sits inside `run()` around `run_inner()` rather than wrapping `run()`, so a cancelled task still reports its terminal `ClientState::Shutdown` / `PortState::Shutdown` to the listener. * TCP sessions are spawned and never joined, so each receives its own clone of the token. The server task returning only drops their command sender, which a session parked mid-write would not observe. * Dropping every handle keeps its previous behaviour of winding down at the next queue poll. Only `shutdown()` is immediate, which leaves callers both a graceful and an abrupt option. The trade-off is that a write already on the wire is abandoned, so the server may still apply it and the caller cannot know. That is inherent to any immediate shutdown and is documented on both methods. `ServerHandle::new()` gains a `CancellationToken` parameter. It is public but documented as existing only for the C bindings, and nothing in the tree calls it. `Channel::shutdown()` and `ServerHandle::shutdown()` landed after the 1.6.0-M2 publish, so no released signature changes.
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
1 participant
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.
Problem
#194 added
shutdown()as a command on the same queue as requests, so it takeseffect only once the task has drained everything ahead of it.
Concretely: a channel parked on an in-flight request waits out the response
timeout. A channel in a retry backoff waits out the delay. A server session
part-way through a write keeps writing.
Shutdown latency is therefore a function of queue depth and the response
timeout, neither of which the caller controls at the point of the call. For a
caller shutting down to release the port, or to exit the process, that is the
wrong dependency.
Change
shutdown()becomes a cancellation signal rather than a queued command:On
ChannelandServerHandleboth. It no longer reaches the task through thequeue, so it no longer reports whether the task was alive, and no longer has to
be awaited.
Each handle shares a
TaskCancellationwith its task —common/cancellation.rs,a wrapper over
tokio_util::sync::CancellationTokenwhoserun_until_cancelledis biased toward cancellation, so a cancelled task is never given another poll of
its work.
Command::ShutdownandServerCommand::Shutdownare gone; the queue carriesrequests and settings again.
ServerCommandandServerHandle::neware no longer public. Both were alreadyunreachable outside the crate —
server::taskhas beenpub(crate) modsince1.5.0 — so this drops dead surface rather than breaking callers. Both
shutdown()methods came from #194, which merged after 1.6.0-M2 shipped, so nopublished signature changes.
Semantics
Client shutdown proceeds in three steps, in this order:
request via
Promise::drop. A write already on the wire may still be applied bythe server, so its outcome is indeterminate.
RequestError::Shutdown, callers blocked on a full queue wake with an error, andanything submitted afterwards fails immediately rather than queueing behind a
task that is going away.
Draining before notifying is the part that matters: a caller awaiting a request
should not be held up by however long a user's listener callback takes.
On the server, the listening socket closes and each session is cancelled at its
next suspension point. Sessions are spawned, so they observe the token directly —
the server task returning only drops their command sender, which a session
part-way through a write would not notice.
Calling
shutdown()twice, or after the task has terminated, does nothing.TCP, TLS and serial clients share
ClientLoopand the samerun()shape, so allthree are covered.
Follow-up
The terminal notification sits outside the cancellation scope deliberately, so
that a cancelled task still reports where it ended up. The cost is an unbounded
await on user code:
Listener::updatereturns aMaybeAsync, and animplementation that never completes parks the task there for good, with
ClientTask::run()never returning and runtime shutdown waiting on it.Pre-existing, and unreachable through the bindings, where every listener returns
MaybeAsync::ready. Bounding it with a timeout and documenting the contract onListener, which currently says nothing about being required to complete, is theintended fix. Can land here or separately.