Repository navigation
Core host API: hand back host completions, TCP connect timeout, single-connection presets - #105
Merged
Merged
Conversation
LocalExecutor::turn drained the loop's output and passed every completion to Shared::dispatch, which returned early for any token without the executor's tag bit. A host that owns the loop and submits its own operations through LocalExecutor::driver therefore never saw their completions: no error, no counter, a socket that simply stopped delivering. That made the asynchronous protocol layers unusable from a host-driven loop. The issue offers two fixes; this takes the first, handing unrouted completions back, because it is the smaller change and composes with a host that already has a dispatch table keyed on the token, with no callback to re-enter the executor from inside its own turn. LocalExecutor::turn_into mirrors Driver::turn: it fills the host's buffer, claims the executor's completions out of it in place (Vec::extract_if, no allocation), and leaves every other completion there in delivery order while the host is free to submit more work. Plain turn keeps those completions in the executor's own buffer, readable through LocalExecutor::unclaimed until the next turn, so it no longer discards anything either. Executor-owned closes used Token(0), which would now surface as foreign completions. They use an internal token carrying the tag with generation zero, which reserve never issues, so it is claimed and matches no slot. Close futures still observe every Closed for their handle, whoever closed it. The contract test drives two real TCP pairs on one loop: the host reads and writes one with its own tokens while an executor task writes and reads the other. Both complete; then the host closes its handles and gets exactly its four Closed back and nothing else. With the old drop-everything dispatch the host read never completes (fails at the 5 s bound with nothing read), and with the internal token reverted to Token(0) the adapters' two closes leak to the host. Closes #45
TcpOpts had only nodelay, so every HTTP client had to build its own connect deadline out of a timer plus close, and nothing said whether the single field was an oversight or a deliberate limit next to Loop::set_option. The issue allows either extending TcpOpts or documenting the boundary; this does both, adding the one field the backends can honour portably. The loop already enforces pipe_connect_until's deadline in the driver, on its own clock: expiry cancels the pending connect and reports TimedOut only after the backend acknowledges the cancellation. TcpOpts::connect_timeout arms that same queue from tcp_connect, so it behaves identically on kqueue, epoll, IOCP and WASI with no backend change. The deadline is retired with the operation, covers the attempt only, and a zero budget is InvalidInput. A local bind address is not added: it would need a bind step in every backend's socket creation, which is a larger change than this issue. The TcpOpts docs now say what is here, what goes through set_option on the handle tcp_connect returns (and that a receive buffer set that way cannot change the negotiated window scale), and that a pre-connect local bind is not offered. Breaking: TcpOpts gains a public field, so struct literals need ..TcpOpts::default(). The two contract uses are updated. The contract test, wired for the Unix, Windows and WASI lanes, checks that a zero budget is refused with nothing retained, that a 5 s budget to a live listener connects and leaves no deadline behind, and that a 1 ms budget that has passed before the first turn completes exactly once with TimedOut and then closes cleanly. Without the deadline insertion the budget is never armed and the late attempt reports Connected. Closes #82
Config's defaults size every table for a server. A client that builds one loop, drives a single outbound connection one request at a time and drops the loop had to trim max_handles and max_operations by hand with no way to know which values were safe, and every such caller guessed independently. Config::single_connection() is that preset: 8 handles, 16 operations, 16 events per turn, two pooled buffers of the default 16 KiB and a post queue of 16. The blocking pool keeps its default because it is process-wide and must match every other loop's. The docs give the floor it covers as a table: the worst moment for this caller is a reconnect (redirect or retry) started before the previous attempt's completions are turned out, which needs 4 handles and 10 operations; the preset leaves headroom above that. They also say which fields fail loudly with ResourceLimit when undersized and which only pace the loop. ExecutorConfig::single_connection() is the executor half: 4 tasks and 8 operation slots, which fits inside the loop preset's 16 operations. Two contract tests exercise the presets. single_connection_config drives the loop directly through that worst moment: a lookup, a connect with a deadline, a read, write and shutdown, then a redirect that closes the socket and its timer, arms a replacement timer, resolves again and reconnects before any of the first attempt's completions are turned out. Nothing is refused with ResourceLimit and the second exchange echoes back byte for byte. It is wired for the Unix and Windows lanes; it needs a threaded std listener as its peer. single_connection_presets carries one request end to end through LocalExecutor under both presets, after abandoning a first adapter with a read pending. The README points one-connection callers at the preset. Closes #81
|
Warning Review limit reachedNext included review available in 41 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (11)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Closes #45, closes #81, closes #82.
Three gaps a host hits when it drives the core loop itself, particularly a short-lived HTTP client loop. There is one commit per issue.
Breaking changes
TcpOptsgains a public field (connect_timeout), so existing struct literals now need..TcpOpts::default(). The contract crate's own uses are updated.LocalExecutor::turnno longer discards completions it does not own. It keeps them, and you can read them throughLocalExecutor::unclaimeduntil the next turn. If you relied on those completions being dropped, you will now see them.New API
LocalExecutor::turn_into(timeout, &mut Completions)works likeDriver::turn. It fills the host's buffer, claims the executor's own completions from it in place (Vec::extract_if, which does not allocate), and leaves every other completion in delivery order.LocalExecutor::unclaimed(): the completions that plainturndid not route.TcpOpts::connect_timeout: Option<Duration>Config::single_connection()andExecutorConfig::single_connection()#45: the host's own completions were dropped
LocalExecutor::turnpassed every completion toShared::dispatch, which returned early for any token without the executor's tag bit. A host that submitted its own operations throughLocalExecutor::drivernever got their completions back. There was no error and no counter; the socket simply stopped delivering. Of the two fixes the issue proposes, this PR takes the first: hand the unrouted completions back. It is the smaller change, and it fits a host that already dispatches on the token.Closes that the executor makes itself used to go out as
Token(0), which would now reach the host as foreign completions. They now use an internal token that carries the tag with generation zero.reservenever issues that generation, so the executor claims these completions and they match no slot.Contract test: two real TCP pairs run on one loop, one driven by the host with its own tokens and one by an executor task. Both complete, and the host gets back exactly its own four
Closedcompletions. Two sabotage checks confirm the test catches the bugs:Token(0)restored, the adapters' closes leak to the host.#82:
TcpOptshad no connect timeoutTcpOpts::connect_timeoutarms the same driver-side deadline queue thatpipe_connect_untilalready uses. When the deadline passes, the loop cancels the pending connect and reportsTimedOutonly after the backend confirms the cancellation. Because the driver enforces it, it behaves the same on kqueue, epoll, IOCP and WASI without any backend change. A zero budget is rejected withInvalidInput.The
TcpOptsdocs now describe the boundary:TcpOptsitself covers;set_optionon the handle thattcp_connectreturns, including that a receive buffer set that way cannot change the negotiated window scale;Contract test (Unix, Windows and WASI lanes):
TimedOutand then closes cleanly.#81: no preset for a one-connection loop
Config::single_connection()sets 8 handles, 16 operations, 16 events per turn, 2 pooled buffers of 16 KiB and a post capacity of 16. The blocking pool keeps its default because it is process-wide.The docs include a table of the worst case this caller meets: a redirect or retry that starts while the previous attempt is still closing. That needs 4 handles and 10 operations. The docs also say which fields fail with
ResourceLimitwhen set too small, and which only affect pacing.ExecutorConfig::single_connection()sets 4 tasks and 8 operation slots, which fits inside the loop preset. The README points one-connection callers at both presets.Contract tests:
single_connection_configdrives the loop directly through the redirect worst case and checks that noResourceLimitoccurs and the second exchange echoes back byte for byte. It runs on the Unix and Windows lanes. It is not wired for WASI because its peer is a threaded std listener.single_connection_presetssends one request end to end throughLocalExecutorwith both presets.Verification (macOS, pinned nightly)
cargo fmt --all,cargo clippy --workspace --all-targets --all-features -D warningsandcargo doc --workspace --no-deps --all-featureswith-D warningsall pass cleanly.cargo test -p turnloop --all-featurespasses.cargo test -p turnloop-contract --all-features -- --test-threads=1: every binary passes except the FSEvents watch tests infilesystem:watch_backpressurefailed on every local run, and fails the same way on unmodifiedorigin/mainon this machine. It is environmental and not caused by this branch, whosedriver.rschange only touchestcp_connect.steady_watch_batches_allocate_nothingfailed intermittently: 1 of the 3 isolated runs passed. It is also FSEvents-timing.cargo +1.97.1 check -p turnloop -p turnloop-contract --all-features --all-targetspasses, includingVec::extract_if. CI checks the rest of the workspace.