Feat/tcp connecotr hardening#179
Conversation
…rt and connection handling improvements
thaodt
left a comment
thaodt
left a comment
There was a problem hiding this comment.
Approved with some small changes then we're good to go.
| aimdb-tokio-adapter = { version = "0.6.0", path = "../aimdb-tokio-adapter", optional = true } | ||
|
|
||
| # test-only (see `_test-embassy-loopback`) | ||
| embassy-net-driver-channel = { path = "../_external/embassy/embassy-net-driver-channel", optional = true } |
There was a problem hiding this comment.
When aimdb-tcp-connector is packaged or published, this dep makes Cargo reject the manifest: cargo package -p aimdb-tcp-connector --allow-dirty --no-verify fails because registry packages require a version and strip path.
You may want to add the matching registry version alongside path or mark the crate non-publishable.
| // Workspace pins `embassy-time` at tick-hz-32_768. | ||
| (start.elapsed().as_micros() * 32_768 / 1_000_000) as u64 |
There was a problem hiding this comment.
With _test-embassy-loopback, no tick-hz-* feature is enabled on embassy-time-driver, so its active fallback is 1000000 Hz rather than 32768 Hz. This advances stack time about 30.5x too slowly, stretching delayed ACK, retransmit and timeout behavior.
Lets derive the scale from embassy_time_driver::TICK_HZ or enable a matching tick feature.
| let mut listener_a = TcpListener::new(server_stack, 7001u16, buf(), buf()); | ||
| let mut listener_b = TcpListener::new(server_stack, 7002u16, buf(), buf()); | ||
| let dialer_a = TcpDialer::new(client_stack, endpoint(7001), buf(), buf()); | ||
| let dialer_b = TcpDialer::new(client_stack, endpoint(7002), buf(), buf()); |
There was a problem hiding this comment.
Lets make this test exercise the pooled-listener path it is intended to cover.
This creates two unrelated TcpListener<1> values on different ports and never calls TcpListener<N>::with_buffers or TcpServer<N>.
The test therefore passes even if same-port fan-out or pooled worker creation is broken and no other in-tree test exercises with_buffers, please use one pooled N=2 endpoint with two clients.
| futures::executor::block_on(async { | ||
| pin_mut!(foreground); | ||
| pin_mut!(background); | ||
| match select(foreground, background).await { |
There was a problem hiding this comment.
Because background is intentionally non-terminating and each foreground accept, connect or recv can stay pending forever, any networking regression or dropped crossover packet leaves block_on running until the outer CI timeout instead of failing the test.
You can race the foreground against a bounded wall-clock watchdog so make test fails promptly.
2 participants
Changes
accept()errors.embassy-net'sTcpSocket::accept()can fail synchronously (a port-0local_endpointrejected asInvalidPort), and theErrarms ofserve_socket_slotand theListener<1>compat path had no await point — a tight loop with zero yields that starves the cooperative executor. Both nowembassy_futures::yield_now().await, degrading a static misconfig to a debuggable warn-loop. New optionalembassy-futuresdep._test-embassy-loopback). The socket pool is welded to a concreteembassy_net::tcp::TcpSocket, so its recycling and waker handoff were review-only. Now exercised over two realembassy-netstacks wired by an in-memory driver-channel crossover: recycle → re-accept, concurrent accept slots, and dialer redial after a failed connect / dropped link. Uses a wall-clock host time driver (the frozen stub clock stalls the delayed-ACK timer and hangs the firstflush()). Kept offembassy-runtimeand out of[dev-dependencies]; wired intomake check.TcpSocketSlotSAFETY comment (dialer/worker/listener-owned, not just "dialer-owned") and documented the intentional double-abort()on the recycle path.aimdb-tcp-connector/CHANGELOG.md(initial) + global changelog index link.