mirror of
https://github.com/rustdesk/hbb_common.git
synced 2026-08-27 04:37:35 +00:00
stream: close the peer connection on drop
A WebRTC peer connection outlives its handle - the session cache holds a clone, and the handler that evicts it only fires on a terminal ICE state - so it has to be closed explicitly. Making that a per-exit-path obligation meant every return, break and `?` had to remember it, and the long-lived side never did: server::connection ends its ~15 exits by dropping the stream, and the transport race drops the losing result outright. Nothing warned; a missed close leaks a pc, its ICE agent and its sockets silently, and only under WebRTC. Stream is not Clone, so dropping it is the end of the transport and there is no second owner to surprise - drop-means-close is simply what the type already meant. The explicit close_webrtc() calls stay valid (they close sooner than scope end), but they are an optimization now rather than the thing correctness rests on. Also delete get_webrtc_stream(): it had no callers and was the one API handing out an owned clone that outlives its Stream, i.e. the only way to defeat this. Regression test included; it fails with the drop body emptied. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ
This commit is contained in:
+15
-7
@@ -203,13 +203,21 @@ impl Stream {
|
|||||||
pub fn from(stream: TcpStream, stream_addr: SocketAddr) -> Self {
|
pub fn from(stream: TcpStream, stream_addr: SocketAddr) -> Self {
|
||||||
Self::Tcp(tcp::FramedStream::from(stream, stream_addr))
|
Self::Tcp(tcp::FramedStream::from(stream, stream_addr))
|
||||||
}
|
}
|
||||||
|
}
|
||||||
|
|
||||||
#[inline]
|
/// Owning the stream owns the transport, WebRTC included.
|
||||||
#[cfg(feature = "webrtc")]
|
///
|
||||||
pub fn get_webrtc_stream(&self) -> Option<webrtc::WebRTCStream> {
|
/// A peer connection outlives its handle — the session cache holds a clone, and the handler that
|
||||||
match self {
|
/// evicts it only fires on a terminal ICE state — so it has to be closed explicitly. Doing that
|
||||||
Self::WebRTC(s) => Some(s.clone()),
|
/// at each exit path made it an obligation every `return`, `break` and `?` had to remember, and
|
||||||
_ => None,
|
/// the long-lived side never did: `server::connection` ends its ~15 exits by dropping the stream.
|
||||||
}
|
/// Nothing warned, because a missed close leaks silently and only under WebRTC.
|
||||||
|
///
|
||||||
|
/// `Stream` is not `Clone`, so dropping it really is the end of the transport and there is no
|
||||||
|
/// second owner to surprise. Explicit `close_webrtc()` calls remain valid — they close sooner
|
||||||
|
/// than scope end — but they are now an optimization rather than the thing correctness rests on.
|
||||||
|
impl Drop for Stream {
|
||||||
|
fn drop(&mut self) {
|
||||||
|
self.close_webrtc();
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -1645,6 +1645,31 @@ IHR5cCBzcmZseCByYWRkciAwLjAuMC4wIHJwb3J0IDY0MDA4XHJcbmE9ZW5kLW9mLWNhbmRpZGF0ZXNc
|
|||||||
panic!("detached close never evicted the peer connection from SESSIONS");
|
panic!("detached close never evicted the peer connection from SESSIONS");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Owning the Stream owns the peer connection: dropping it must close the pc and evict the
|
||||||
|
// session, without the owner having to remember to. Every exit path used to carry that
|
||||||
|
// obligation, and the controlled side never honoured it.
|
||||||
|
#[tokio::test]
|
||||||
|
async fn dropping_the_stream_closes_the_peer_connection() {
|
||||||
|
let offerer = WebRTCStream::new("", false, 20000).await.unwrap();
|
||||||
|
let key = format!("offer:{}", offerer.session_key());
|
||||||
|
assert!(
|
||||||
|
SESSIONS.lock().await.contains_key(&key),
|
||||||
|
"offerer should be cached while live"
|
||||||
|
);
|
||||||
|
|
||||||
|
// No explicit close anywhere: the stream simply goes out of scope, as it does on the
|
||||||
|
// exits that forget.
|
||||||
|
drop(crate::Stream::WebRTC(offerer));
|
||||||
|
|
||||||
|
for _ in 0..200 {
|
||||||
|
if !SESSIONS.lock().await.contains_key(&key) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
tokio::time::sleep(Duration::from_millis(50)).await;
|
||||||
|
}
|
||||||
|
panic!("dropping the stream left the peer connection in SESSIONS");
|
||||||
|
}
|
||||||
|
|
||||||
// A replayed offer asking for a different ICE policy must not be handed the cached peer
|
// A replayed offer asking for a different ICE policy must not be handed the cached peer
|
||||||
// connection. The lookup and the insert-time duplicate check read the same key, so the test
|
// connection. The lookup and the insert-time duplicate check read the same key, so the test
|
||||||
// fails if either one stops applying `is_reusable_for` — which is how the guard was dead
|
// fails if either one stops applying `is_reusable_for` — which is how the guard was dead
|
||||||
|
|||||||
Reference in New Issue
Block a user