Repository navigation
fix(transport): stop listener accept loops when the transport layer is cancelled - #206
Conversation
…s cancelled The TCP, TLS and WebSocket listener accept loops ran in detached tasks that never observed the transport layer's cancel token, so a cancelled transport layer kept its listening sockets bound and kept TransportLayerInner (and its connection map) alive. Race accept() against the token, as the UDP serve loop already does, and exit the loop on cancel.
tgeorge06
left a comment
There was a problem hiding this comment.
Thanks for this, nice catch. I pulled the branch and checked it locally against current main (3187537, merges cleanly):
- New tests red/green: on
mainwith onlytest_listener_api.rsapplied,test_tcp_listener_closed_on_cancelandtest_websocket_listener_closed_on_cancelboth fail ("still accepting after cancel"); on this branch both pass. cargo test --features benchand plaincargo test: all green (441 lib + 65).cargo check --no-default-features --features platform-embassy: clean, no new warnings (the listeners are all behindplatform-tokio).cargo clippy --features bench --all-targets: identical output tomain(same pre-existingnever_looperror intest_refer_notify.rs, no new lints in the touched files).cargo fmt --checkclean.
Behaviour I checked by hand:
- Only the accept loop stops. Already-accepted connections keep running until the layer's cancellation reaches them through their child tokens and
serve_connection, same as before. I confirmed with a scratch test that an accepted TCP connection stays open beforecancel()and the client sees EOF after it. select2polls the cancel arm first, so a connection that is ready at the same moment as cancel is not handed off. A TLS/WS handshake already in flight can still finish after cancel, but its child token is already cancelled, soserve_connectionretires it right away. Nothing leaks in the map.- The listening socket is really released: after cancel, a plain
std::net::TcpListener::bindon the same port succeeds. - Accept errors still
warn!and continue as before. UDP already races its recv against the token, and WSS goes through the same WebSocket loop, so I don't think any listener was missed.
One small gap: tls.rs changes but has no regression test. Here is one that mirrors the TCP/WS ones and reuses the existing cert helper. It fails on main and passes here (3/3 runs):
// src/transport/tests/test_listener_api.rs
#[cfg(feature = "rustls")]
#[tokio::test]
async fn test_tls_listener_closed_on_cancel() -> Result<()> {
use crate::transport::{TlsConfig, TlsListenerConnection};
let (cert, key) = super::test_tls_reload::generate_test_cert("localhost")?;
let config = TlsConfig {
cert: Some(cert.into_bytes()),
key: Some(key.into_bytes()),
..Default::default()
};
let addr = free_port();
let listener = TlsListenerConnection::new(addr, None, config).await?;
assert_listener_closed_on_cancel(listener.into(), addr).await;
Ok(())
}It needs the helper made visible to the sibling module:
--- a/src/transport/tests/test_tls_reload.rs
+++ b/src/transport/tests/test_tls_reload.rs
#[cfg(feature = "rustls")]
-fn generate_test_cert(subject_cn: &str) -> std::result::Result<(String, String), std::io::Error> {
+pub(super) fn generate_test_cert(
+ subject_cn: &str,
+) -> std::result::Result<(String, String), std::io::Error> {Happy to open a small follow-up PR with the TLS test if you'd rather merge this as is.
Not for this PR, just something I noticed in the same area: the TLS and WebSocket handshake tasks have no timeout and don't watch the token, so a peer that connects and then stalls the handshake keeps a TransportLayerInnerRef alive after cancel. That was already the case on main, but it is the same kind of per-session leak, so it may be worth a follow-up.
LGTM.
Test suggested by @tgeorge06 in review.
|
Thanks for the thorough review! I added your TLS test in 61a6762. Agreed on the handshake timeout, that's worth investigating! |
Summary
The TCP, TLS and WebSocket listener accept loops run in detached tasks that never observe the transport layer's cancel token. Cancelling the token stops connections and the endpoint, but each listener keeps its socket bound and keeps
TransportLayerInneralive, along with every connection in its map.Problem
An application that creates a
TransportLayerper session (e.g. one SIP stack per call) leaks the listening socket plus the session's stream connections on every session, even after cancelling the token. In a long-running daemon we measured +3 file descriptors and ~230 KB per call, never reclaimed, until the process hits its fd limit.Changes
accept_or_cancelledhelper racesaccept()against the cancel token usingplatform::select::select2, mirroring the UDP serve loop.TransportLayeris still alive.Compatibility
No API change. Listeners already stop when the process exits, they now also stop when their transport layer's token is cancelled.