Repository navigation
fix(socket/tcp): do not abort idle connections on user timeout - #1204
Open
Aditya-9-6 wants to merge 1 commit into
Open
Aditya-9-6 wants to merge 1 commit into
Aditya-9-6 wants to merge 1 commit into
Conversation
Per RFC 5482 (TCP_USER_TIMEOUT) and smoltcp's documented set_timeout contract, user timeout bounds how long transmitted data may remain unacknowledged, or how long an unresponsive peer is tolerated during keep-alive probing or handshake/teardown. A quiescent, healthy connection with an empty transmit buffer should not be aborted. Previously, poll_at() unconditionally armed remote_last_ts + timeout even when the connection was completely idle, and timed_out() aborted connections after the timeout window elapsed without traffic. Additionally, close() from an idle connection retained stale idle timestamps, which could trigger premature abortion of the in-flight FIN. This change: - Introduces timeout_active() to verify whether the socket is in a state actively awaiting remote response (SYN/FIN in flight, pending data in transmit buffer, or keep-alive enabled). - Updates timed_out() and poll_at() to return PollAt::Ingress when idle rather than scheduling an abort timer. - Clears remote_last_ts on close() when transitioning to FinWait1 or LastAck so FIN timeouts are counted from FIN transmission. - Restores test_established_timeout to assert PollAt::Ingress while idle and adds regression tests for extended idle periods and close timeouts. Fixes smoltcp-rs#1202.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1204 +/- ##
==========================================
+ Coverage 82.57% 82.60% +0.02%
==========================================
Files 83 83
Lines 26174 26215 +41
==========================================
+ Hits 21613 21654 +41
Misses 4561 4561 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Summary
Fixes #1202.
Per RFC 5482 (
TCP_USER_TIMEOUT) and smoltcp's documentedset_timeoutcontract, user timeout bounds how long transmitted data may remain unacknowledged, or how long an unresponsive peer is tolerated during keep-alive probing or handshake/teardown:RFC 5482 Section 2 states:
Root Cause
poll_at(),timeout_poll_atunconditionally armedPollAt::Time(remote_last_ts + timeout)wheneverremote_last_tsandtimeoutwere present, even when the socket was inEstablishedwith an empty transmit buffer and no keep-alive.timed_out(), expiration was evaluated solely astimestamp >= remote_last_ts + timeout, causing connections to abort with RST aftertimeoutof quiet.close(), initiating a FIN on a connection that had been idle for an extended period inherited the staleremote_last_ts, which could trigger an immediate/premature timeout before the remote peer could ACK the FIN.Solution
timeout_active(&self) -> bool: Added helper to determine whether the socket is in a state actively awaiting a remote peer response:SynSent|SynReceived: Connection handshake active.FinWait1|Closing|LastAck: Connection teardown active (waiting for FIN ACK).Established|CloseWait: Active if!self.tx_buffer.is_empty()(data in flight) ORself.keep_alive.is_some()(keepalive probing active).poll_at: ReturnsPollAt::Ingressfor idle connections, and only queriesPollAt::Nowfor timestamp initialization whentimeout_active()is true.close(): Clearsself.remote_last_ts = Nonewhen transitioning toFinWait1orLastAckso the FIN timeout begins freshly upon FIN transmission.test_established_timeoutto assertPollAt::Ingresswhile idle before data is queued.test_established_idle_no_timeoutverifying extended quiet periods do not abort established connections.test_established_idle_close_timeoutverifyingclose()after extended idle periods does not prematurely abort the FIN handshake.