Update ioxide to 0.14.239 and pin the new TCP timeouts - #916
Merged
Merged
Conversation
ioxide 0.14.236 gave TcpOptions an idle and a send timeout, both defaulting to 60s. Taking that default silently would start closing idle websockets: this listener carries upgraded connections as well as keep-alive HTTP, and the transport cannot tell them apart. An idle websocket is a healthy one. So both are exposed on TcpTransportOptions and wired through, defaulting to 0 - disabled. The bump is therefore behaviour-neutral, and the capability becomes configurable rather than inherited. The defaults are off rather than "sensible" on purpose: IdleTimeoutMs closes a connection quiet in both directions. Right for HTTP keep-alive, wrong for a websocket whose ping interval exceeds it. A server with no upgrades can turn it on; one with upgrades should set it above the ping interval so pings keep the connection marked alive. SendTimeoutMs bounds a client that stops reading - its window shuts, the send never completes, and nothing in TCP ends that. But while ioxide sends with MSG_WAITALL it is a deadline for the WHOLE response, not for progress within it, so a large download to a slow client counts against it. That wants sizing against the slowest legitimate response, which is a per-deployment number. What 0.14.239 brings that matters here: recv buffers a reader still holds are reclaimed at teardown. TcpDriver.CloseAsync already completes both halves before DecRef, so the common path was never leaking - but completing does not disarm a read that is still parked, and items ingested after that had nobody left to return them. That case needed no mistake by the caller. Full solution builds clean, 0 warnings. The acceptance suite passes everything it runs and then aborts on io_uring_setup - which reproduces identically on unmodified main, at a different point each time, so it is this machine's io_uring limits rather than anything in this change.
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.
Bumps all six
ioxidepackage references from0.14.236to0.14.239, and pins two new transport options so the bump does not change behaviour on its own.Why this is not just a version bump
ioxide
0.14.236gaveTcpOptionsan idle timeout and a send timeout, both defaulting to 60 seconds. Before that, a TCP connection had no clock at all.Server.WithTcpmaps six TCP knobs and set neither, so simply moving the version would have inherited both defaults — and this listener carries upgraded connections as well as keep-alive HTTP. A websocket that goes quiet in both directions for a minute is a healthy websocket, and the transport cannot tell it apart from a dead one. Only a ping can, and that is the application's business.So both are now on
TcpTransportOptionsand wired through, defaulting to0(disabled). The bump is behaviour-neutral, and the capability is configurable rather than inherited.Why the defaults are off rather than "sensible"
IdleTimeoutMs— closes a connection quiet in both directions. Correct for HTTP keep-alive, wrong for a websocket whose ping interval is longer than it. A server with no upgrades can turn it on; a server with upgrades should set it comfortably above the ping interval, so pings keep the connection marked alive.SendTimeoutMs— bounds a client that stops reading. Its receive window shuts, the send never completes, and nothing in TCP ends that: a zero window is a legitimate state a peer can hold indefinitely. Genuinely worth having. But while ioxide sends withMSG_WAITALLit is a deadline for the whole response rather than for progress within it, so a large download to a slow client counts against it. That number depends on what you serve, so it is not one this project should pick for you.Both are documented in place with that reasoning.
What the ioxide release brings
Recv buffers a reader still holds are now reclaimed at connection teardown.
TcpDriver.CloseAsyncalready completes both halves beforeDecRef, so the common path here was never leaking.It still matters, though: completing a reader does not disarm a read that is still parked. The awaiter stays armed, the next completion resumes it, and items ingested after that had nobody left to return them. That case needs no mistake by the caller, and it is the shape a TLS teardown produces.
Also in the release: the version constant is generated from the csproj rather than hand-kept, XML docs now ship with the packages, and
TcpConnectionStreamfinally has aDisposethat returns its buffer — which matters to anyone wrapping it in anSslStream.Verification
Full solution builds clean, 0 warnings, 0 errors.
The acceptance suite passes everything it runs — 1,181 / 1,199 across the two target frameworks on one pass, 756 / 810 on another — and then aborts with
io_uring_setup failed: -1. That reproduces identically on unmodifiedmain, at a different point each run (it got to 2,130 there), so it is this machine's io_uring instance limits rather than anything in this change. Zero test failures on either branch; I could not get a complete local run on either.Housekeeping
The
update-ioxide-0-14-236branch is superseded by this one and can be deleted —mainis already on0.14.236, so its commit is a no-op now.