Skip to content

fix: retire dead stream connections instead of reusing them - #161

Open
tgeorge06 wants to merge 1 commit into
restsend:mainfrom
tgeorge06:fix/retire-dead-stream-connections
Open

tgeorge06 wants to merge 1 commit into
restsend:mainfrom
tgeorge06:fix/retire-dead-stream-connections

Conversation

@tgeorge06

Copy link
Copy Markdown

Fixes #160.

Problem

TransportLayer caches stream connections (TCP/TLS/WS) by remote address. When a connection's serve loop ends (the peer closed it, e.g. a carrier or proxy idle-closing the flow), the connection stays in the cache and its cancel token is never cancelled. So:

  • lookup() keeps returning the closed connection for every later request to that peer;
  • dialog flow affinity (resolve_affinity_connection, which skips flows whose token is cancelled) keeps choosing it for in-dialog requests;
  • Transaction::send() only logs the write error and, because a reliable transport arms no Timer A, the request is never retransmitted or sent on a new connection. It waits for Timer B/F and gets a local 408: 32 s per request, for every request to that peer, until the application calls del_connection itself.

Spec

  • RFC 3261 §18.1.1 / §18.4: a request is sent on an existing connection to the destination, or a new one is opened. A connection the peer closed is not an existing connection.
  • RFC 3261 §17.1.4: a transport failure is reported to the TU at once; §8.1.3.1: the TU treats it as a 503.

Fix

  • serve_connection: when a stream connection's serve loop exits, cancel its token (flow affinity then skips it, as its comment intends) and remove it from the cache, before awaiting close(). Removal is by identity (Arc::ptr_eq), so a newer connection cached for the same peer is kept.
  • Transaction::send() / Timer A resend: when the write on a stream connection fails, retire the connection the same way and report a local 503 to the TU, instead of waiting for Timer B/F. The transaction does not re-send the request itself (the write may have partly gone out); the next request opens a new connection.
  • UDP and channel connections are unchanged. No public API change; the new helpers are crate-private (SipConnection::is_stream, SipConnection::is_same_stream, TlsConnection::ptr_eq, TransportLayer::retire_connection).

Contract / coverage

  • Transports: TCP, TLS and WS/WSS connections all go through add_connection → serve_connection, and is_stream / is_same_stream cover all three (TLS client and server sides). The tests use TCP.
  • Roles: the serve-loop retirement applies to every cached stream connection, outgoing or accepted, so UAC and UAS dialogs both stop reusing a flow once the peer has closed it. The immediate 503 applies to client transactions (initial requests and in-dialog requests alike).
  • Flow affinity (RFC 5626 / RFC 7118): a retired flow's token is cancelled, so resolve_affinity_connection falls back to normal resolution or the Via dial-back instead of re-selecting it (tested: the token is cancelled when the peer closes the flow).
  • What the caller sees: the 503 is built and delivered like the stack's existing local 408 for Timer B/F (make_response from the request, through the transaction's normal response path). For an INVITE the transaction then handles it like any non-2xx final response.
  • Timers: a write failure no longer waits for Timer B/F; with no write failure nothing changes.

Tests

New src/transaction/tests/test_stream_reconnect.rs (raw TCP peer; the first two use only Transaction::new_client / send / receive):

  • test_request_after_peer_closed_stream_uses_new_connection: the peer closes the connection after the first OPTIONS; the flow is marked terminated, and the next OPTIONS goes out on a new connection and is answered.
  • test_send_failure_on_stream_is_reported_at_once: OPTIONS and INVITE on a connection whose writes fail get a 503 at once, and that connection is retired.
  • test_retire_keeps_newer_connection_to_same_peer: retiring an old connection keeps a newer one cached for the same peer; a retired connection is not returned by lookup.

Checks (on main @ 3ea35cd)

  • Red: with the src/ change reverted and the two public-API tests kept (the third uses the new helpers and does not compile without them), cargo test --lib test_stream_reconnect → 2 failed (a flow the peer closed must be marked terminated: Elapsed(()); a failed OPTIONS write on a stream connection must be reported to the TU, not left to time out, left: None).
  • Green: cargo test → 335 lib + 65 doc passed, 0 failed. rustfmt --check clean on the changed files. The new tests passed 10 repeated runs.

Out of scope

  • Write failures on server transactions (responses), ACK and CANCEL do not retire the connection yet; the serve-loop exit still retires it once the peer's close is seen.
  • TLS and WebSocket end-to-end tests.

The transport layer caches stream connections (TCP/TLS/WS) by remote
address but never removed one when its serve loop ended, and nothing
cancelled its token. A connection the peer closed (a carrier or proxy
idle-closing the flow) was therefore handed out by lookup() for every
later request to that peer, and dialog flow affinity kept choosing it
for in-dialog requests. Since send() logs write errors and the
transaction has no Timer A on a reliable transport, each such request
sat until Timer B/F and then got a local 408: 32 s per request, for
every request to that peer, for good.

When a stream connection's serve loop exits, cancel its token and drop
it from the cache (only if the cached entry is that same connection).
When a write on a stream connection fails, retire it the same way and
report the failure to the TU as a local 503 (RFC 3261 §17.1.4,
§8.1.3.1) instead of waiting for the timeout. Later requests then open
a new connection. UDP and channel connections are unchanged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A stream connection the peer closed stays cached and every later request waits for Timer B/F

1 participant