Repository navigation
feat(net): header and idle timeouts on peer streams - #119
Conversation
AlxcBTW
left a comment
There was a problem hiding this comment.
Thanks fago, the header timeout and the two silent-peer tests are the right shape, and resetting the stream on a bad header is good. Three things before merge, mostly about not dropping a live peer:
pipe: the activity flag is only set when bytes are read, before the write. If the other side is slow andwrite_allblocks for longer than the idle time, the watchdog sees no activity and kills a stream that is still moving data. Please also mark activity after each completed write (or count progress on both the read and the write).- 60 s with nothing in either direction is short for a live game connection that is simply quiet (menus, lobbies, waiting for a turn). Please either raise
STREAM_IDLE_TIMEOUTa lot (several minutes) or tell me why 60 s is safe for games; I would rather the tunnel keep a quiet stream than drop it. A short comment next to the constant on the reasoning is enough. - Please add a test that a slow but live stream survives: send one byte every ~idle/3 for a few multiples of the idle time and assert the stream is still open and the bytes arrive. Also
open_stream_with(bridge side, silent TCP client sends no header) has no test; one small one would do.
Nit: read_head.is_err() || read_head.unwrap().is_err() reads better as a match or !matches!(read_head, Ok(Ok(_))).
2b685e3 to
8532f81
Compare
|
Thanks for the thorough review and guidance @AlxcBTW! I have addressed all feedback in the latest commit:
All unit tests and workspace clippy / fmt checks pass cleanly. |
AlxcBTW
left a comment
There was a problem hiding this comment.
All three points are in: activity marked after each write, 300 s idle with the reasoning, and the slow-but-live and silent-bridge tests. Thanks fago, merging.
|
Merged. Thanks for the quick turnaround, fago. |
What and why
When a peer opens a bidirectional stream via
deliver_stream, it previously waited indefinitely for the 4-byte tunnel header (vportandsource_vport). If a peer opened a stream and stayed silent, the task and stream resources hung indefinitely. Additionally, once established, the bidirectional pipe had no idle timeout, leaving inactive streams hanging if neither side transferred data.This PR introduces:
STREAM_HEADER_TIMEOUT(5 seconds) to bound the wait for tunnel headers on peer-initiated streams. Silent peers that do not deliver the header in time are reset and dropped.STREAM_IDLE_TIMEOUT(300 seconds / 5 minutes) to detect when bidirectional streams have no transfer in either direction and terminate idle connections cleanly, with ample headroom for quiet menus and game lobbies.pipe()to ensure slow sinks moving data do not trigger false watchdog timeouts.silent_peer_stream_header_times_outsilent_peer_stream_idle_times_outslow_live_stream_survives_idle_timeout(verifying periodic low-frequency writes survive across multiples of idle timeout)silent_bridge_stream_header_times_out(verifying bridge drops silent loopback TCP clients failing to send traffic headers)STREAM_HEADER_TIMEOUTandSTREAM_IDLE_TIMEOUTfromtunnel.rs.Closes #
Checklist
cargo fmt --all -- --check,cargo clippy --workspace --all-targets -- -D warningsandcargo test --workspacepass.docs/adr/.spec/bridge-protocol.md,crates/bridge-protocol) states its wire effect: the version, and whether the AnyPS5 fork has to change with it.