sui-rpc: better defaults to avoid http2 stream starvation#284
Merged
Conversation
h2 versions before 0.4.14 have several stream-cancellation flow-control accounting bugs (hyperium/h2#893, #896, #897, #898, and #913, fixed across 0.4.14 and 0.4.15) that can permanently wedge a multiplexed gRPC connection: leaked connection send-window capacity, RST_STREAM frames that are never transmitted, and permanently missed `poll_capacity` wakeups. Add an explicit hyper 1.10 floor to sui-rpc (hyper 1.10 raised its h2 floor to 0.4.14 for the same reason) so downstream consumers cannot resolve a broken h2. The lockfile is not tracked in this repo, so the manifest floor is the durable guard.
Every RPC made through a Client (and all of its clones) is multiplexed over a single HTTP/2 connection, so the connection-level receive window is shared by all in-flight responses. A streaming response that the application holds without polling pins up to a full stream window of that shared budget, and with hyper's client defaults (2 MiB stream / 5 MiB connection) about three stalled streams exhaust the connection window. From then on every RPC on the channel hangs indefinitely -- response HEADERS are not flow-controlled and still arrive, but response DATA can never be delivered -- while TCP and HTTP/2 keepalives stay healthy, so nothing times out. Client::new now sets the per-stream window to 2 MiB explicitly and raises the connection window to 64 MiB, so ~32 concurrently stalled streams are needed to starve the connection instead of ~3. Add with_initial_stream_window_size and with_initial_connection_window_size overrides, and document that Client::from_endpoint bypasses these defaults and that http2_adaptive_window is not a substitute (adaptive mode starts the connection window at the 64 KiB spec default, making starvation worse).
Raising the flow-control windows makes the shared connection hard to starve, but a response stream that the application holds without polling still pins its window forever, and an RPC whose response can no longer make progress still hangs forever. Nothing in the default stack bounds either: HTTP/2 keepalive PINGs are answered by the peer's transport layer even when every stream is stalled, so the connection looks healthy throughout. Add a watchdog layer, on by default, applied inside Client::channel() beneath any user request layers. It moves each response body into a spawned task bridged back to the caller through a bounded mpsc(1) channel. The task's outermost await is an idle timer (default 30 seconds, comfortably above the fullnode's ~5 second subscription watermark cadence), so it stays responsive even when the caller has parked the response and nothing is polling it. A poll-driven timer would not work here: a parked stream is never polled, which is exactly when the timer is needed. On expiry the task drops the inner body, which sends RST_STREAM (a control frame, deliverable even on a starved connection) and releases all of the flow-control window the stream had pinned; the caller observes DeadlineExceeded on its next poll. Dropping the response aborts the task via an abort-on-drop guard, and a panic in the task surfaces as an Internal status rather than a clean end-of-stream. The timeout is configurable with Client::with_body_idle_timeout, disableable with Client::without_body_idle_timeout, and overridable per call (including re-enabling or disabling) by inserting a BodyIdleTimeout extension into the request.
Previously no per-call deadline was enforced end to end. tonic's channel machinery parses the grpc-timeout header that tonic::Request::set_timeout attaches, but only enforces it until response headers arrive. On a starved connection that is precisely the part that still works -- headers are not flow-controlled -- so the deadline never bounded the actual hang: unary response DATA that can never be delivered, or a streaming body that outlives its budget. The watchdog task now also enforces the request's grpc-timeout deadline across the whole response body, measured from request time so the budget communicated to the server is the same one enforced locally. On expiry the body is dropped (resetting the stream and releasing its pinned flow-control window) and the call observes DeadlineExceeded. This makes Request::set_timeout a total per-call deadline: the server participates through the standard header once it honors it, and the client enforces it locally regardless. Also document the deadline story on Client.
Port the standalone reproduction of the indefinite gRPC hang as permanent regression tests. A mock fullnode built from this crate's own generated server stubs pushes large checkpoint-subscription frames as fast as the transport accepts them, and the tests assert each layer of the defense independently: - a control test with the pre-fix configuration (hyper's default 2 MiB stream / 5 MiB connection windows via Client::from_endpoint, watchdog disabled) reproduces the hang with four parked subscription streams and recovers when they are dropped, proving the harness creates real starvation pressure; - the Client::new window defaults absorb eight parked streams while unary calls and an actively polled subscription keep working, with the watchdog disabled to show the windows alone carry the load; - the idle-body watchdog reaps parked streams on the small-window configuration, un-sticking the connection without any help from the caller, and the parked calls observe DeadlineExceeded when finally polled; - tonic::Request::set_timeout cuts a streaming call at its deadline while items are flowing.
Previously the watchdog task pulled a frame from the transport and then awaited sending it into the bridge channel, so a parked consumer left one frame buffered in the channel plus one held in limbo by the blocked send. Pulling a frame out of the body also releases its HTTP/2 receive-window capacity, inviting the server to send more data than the consumer is actually accepting. Reserve a permit with Sender::reserve before polling the body, so a frame is only taken from the transport once it is immediately deliverable. A parked consumer now buffers at most one frame in the bridge, everything else stays in the transport where it remains subject to flow control, and a dropped consumer is detected before pulling another frame rather than after. The idle-timer semantics are unchanged: the timer still covers both awaits, firing whether the consumer is parked (reserve never resolves) or the transport is starved (no frame arrives). Add a test pinning the no-double-buffering property.
Previously the client had no way to bound every RPC by default: a deadline had to be set per request with `tonic::Request::set_timeout`, and a call against a server that never responded could wait on response headers indefinitely (headers are not flow-controlled, so neither the flow-control windows nor the idle-body watchdog bound that phase). With this commit, `Client::with_response_headers_timeout` exposes `tonic::transport::Endpoint::timeout`, which tonic enforces locally from request dispatch until response headers arrive. Because a server does not send response headers for a unary call until the handler completes, this effectively bounds the total duration of unary calls, while the timer is dropped once headers arrive, so a client-wide value does not cut off long-lived streaming responses. The timeout is not communicated to the server, and tonic combines it with a per-call `grpc-timeout` deadline by taking the shorter of the two, so a per-call deadline can tighten the local bound but never extend it. Also normalize tonic's `TimeoutExpired` error, which tonic surfaces as `Cancelled`, to `DeadlineExceeded` in the client's error mapping. The gRPC code for an expired deadline is `DeadlineExceeded`, and the watchdog already uses it for body-phase expiry, so both phases of a deadline now report the same code. Add integration tests covering a stalled unary call failing at the timeout with `DeadlineExceeded`, the shorter-of-the-two interaction with per-call deadlines in both directions, a subscription stream outliving the timeout, and a healthy call passing through unaffected.
Previously the list_owned_objects, list_dynamic_fields, list_balances, and list_package_versions streams treated a page with zero items as end-of-stream even when the response carried a next_page_token. The list-pagination contract allows such pages (for example, a filtered scan that hit a server-side budget before finding a match), so the streams would silently drop every remaining page -- data loss with no error. The list_delegated_stake helper already handled this correctly by looping on the token; the four stream helpers were the outlier. With this commit the streams keep following the token until a page yields an item or pagination ends. Also add integration tests against a scripted mock server covering an empty middle page and a mid-pagination error; the empty-page test fails against the previous behavior.
execute_transaction_and_wait_for_checkpoint subscribed to the checkpoint stream and then held it unpolled while awaiting ExecuteTransaction and GetTransaction on the same channel -- the hold-and-wait shape at the root of the connection-starvation hangs. Checkpoints keep arriving whether or not anyone reads them, so the parked subscription pinned up to a stream window of the shared connection receive window for the whole execution phase, and once the execution phase outlasted the client's idle-body watchdog the subscription was reset, turning a successful execution into a spurious CheckpointStreamError. Restructure the method to scan the subscription concurrently with the execution phase: the digest is computed from the request up front (so an invalid request now also fails before any network work), and a select loop drives the execute-then-GetTransaction future and the digest scan together. Semantics are preserved: the subscription is opened before execution so the checkpoint cannot be missed, the GetTransaction duplicate-submission shortcut still takes precedence, and the wait timeout still covers only the confirmation phase. A scan result that arrives mid-execution (for example, a duplicate of an already executed transaction landing in a checkpoint) is remembered and used once execution completes. Add regression tests against a mock fullnode: a slow execution with a short watchdog (fails as CheckpointStreamError against the previous implementation), the duplicate-submission shortcut, and fail-fast validation of a request with no transaction.
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.
No description provided.