Hoist tokio spawn by JustinKovacich · Pull Request #80 · luminartech/simple_someip

JustinKovacich · 2026-04-23T00:28:05Z

Swap out the old announcement loop.

This PR is part of a chain. See Prev: #79; See Next: #81

Copilot

Pull request overview

This PR hoists internal tokio::spawn calls out of the client and server APIs by returning caller-driven run-loop futures (client run loop + server SD announcement loop), and updates tests/docs to match.

Changes:

Replace server start_announcing() with announcement_loop() that returns a future to be spawned/driven by the caller.
Change Client::new / new_with_loopback to return (Client, ClientUpdates, run_future) instead of spawning internally.
Update integration/unit tests and documentation to spawn the returned futures.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
`tests/client_server.rs`	Updates integration tests to spawn the client run-loop future returned by `Client::new`.
`src/server/mod.rs`	Replaces `start_announcing` with `announcement_loop` returning a future; updates/extends server tests.
`src/server/README.md`	Updates server docs to show spawning the announcement-loop future.
`src/lib.rs`	Updates crate-level docs to reflect new `Client::new` return shape and spawned run future.
`src/client/mod.rs`	Changes `Client::new`/`new_with_loopback` API to return a run future; adds tests documenting lifecycle pitfalls.
`src/client/inner.rs`	Refactors inner loop to return a run future rather than spawning internally; updates tests accordingly.
`examples/client_server/src/main.rs`	Updates comments for server announcement API, but currently still uses the old `Client::new` signature.
`README.md`	Updates server quick-start to use `announcement_loop`, but client quick-start remains on the old `Client::new` signature.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Addresses the six open comments on PR #80 that 76ee480 did not cover; the three structural decisions (Client::new breaking signature, start_announcing -> announcement_loop rename, panic -> typed error) were all confirmed as "break, no wrapper" by the maintainer. Typed Error::Shutdown (comment #80-6) ===================================== Added `Error::Shutdown` to `client::Error` — the variant the run-loop control channel surfaces when its receiver is gone (run future dropped, cancelled, or exited). Converted every public Client method that routes through `control_sender` to return `Err(Error::Shutdown)` on channel closure instead of panicking on `.unwrap()` of the send or recv result: - 9 call sites in client/mod.rs switched from self.control_sender.send(..).await.unwrap(); response.await.unwrap() to self.control_sender.send(..).await.map_err(|_| Error::Shutdown)?; response.await.map_err(|_| Error::Shutdown)? - `request()`'s `response_rx.await.expect(...)` follows the same pattern. - `PendingResponse::response()`'s `.expect(...)` does too. Rewrote the two `#[should_panic]` regression tests that were locking in the panic behavior: - `dropping_run_future_without_spawn_returns_shutdown_error` - `cancelling_run_future_closes_control_channel_returns_shutdown_error` Both now assert `matches!(err, Error::Shutdown)` after the expected drop/abort, so any regression that reintroduces the panic path fails the test on the panic itself and any regression that reverts the typed error fails on the matches! check. Removed the `# Panics` doc sections from affected methods; `set_interface` keeps a `# Panics` note scoped to the interface-lock-poisoning case, which is unrelated to the control channel. The new variant's doc comment names the lifecycle condition that produces it. Test-hygiene fixes (comments #80-4, #80-5, #80-10) ================================================== - `drop(tokio::spawn(fut))` at two test sites (server/mod.rs lines 1310 and 1956) replaced with `drop(fut)`. Spawning + detaching the JoinHandle left the announcer task alive for the rest of the test binary's lifetime, emitting multicast that could confuse sibling tests bound to the same group. The test only cares that construction returned Ok, so don't poll the future at all. - `announcement_loop_sends_offer_service_when_driven` strengthened from "message_id == SD" to parsing the SD header, filtering for an `OfferService` entry matching the server's configured service_id / instance_id, and asserting major_version plus a non-zero TTL. Matches the pattern used by the sd_state.rs multicast tests (dedicated service_id + recv loop filter) so parallel test traffic doesn't cause false matches. Docs (#80-3, #80-7, #80-8) ========================== - src/lib.rs: the phase-6 quickstart docs claimed the client run-loop future "can be spawned on any executor", but it depends on tokio (select!, time, sockets). Tightened to "spawn on the tokio runtime" plus a one-line rationale. - examples/client_server/src/main.rs and examples/discovery_client/ src/main.rs: both still destructured the old 2-tuple from `Client::new`. Updated to destructure the 3-tuple and spawn the run future. They were already failing `cargo check --workspace`. - README.md: same fix, with explicit wording that not spawning the run future produces `Error::Shutdown` on subsequent client calls. Verification: `cargo test --all-features --lib` 426/427 pass. The one failure (`announcement_loop_sends_offer_service_when_driven`) is a pre-existing multicast-loopback environmental issue on this machine — reproduces on the 76ee480 base without any of these changes. CI will validate it there. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Addresses the six open comments on PR #80 that 76ee480 did not cover; the three structural decisions (Client::new breaking signature, start_announcing -> announcement_loop rename, panic -> typed error) were all confirmed as "break, no wrapper" by the maintainer. Typed Error::Shutdown (comment #80-6) ===================================== Added `Error::Shutdown` to `client::Error` — the variant the run-loop control channel surfaces when its receiver is gone (run future dropped, cancelled, or exited). Converted every public Client method that routes through `control_sender` to return `Err(Error::Shutdown)` on channel closure instead of panicking on `.unwrap()` of the send or recv result: - 9 call sites in client/mod.rs switched from self.control_sender.send(..).await.unwrap(); response.await.unwrap() to self.control_sender.send(..).await.map_err(|_| Error::Shutdown)?; response.await.map_err(|_| Error::Shutdown)? - `request()`'s `response_rx.await.expect(...)` follows the same pattern. - `PendingResponse::response()`'s `.expect(...)` does too. Rewrote the two `#[should_panic]` regression tests that were locking in the panic behavior: - `dropping_run_future_without_spawn_returns_shutdown_error` - `cancelling_run_future_closes_control_channel_returns_shutdown_error` Both now assert `matches!(err, Error::Shutdown)` after the expected drop/abort, so any regression that reintroduces the panic path fails the test on the panic itself and any regression that reverts the typed error fails on the matches! check. Removed the `# Panics` doc sections from affected methods; `set_interface` keeps a `# Panics` note scoped to the interface-lock-poisoning case, which is unrelated to the control channel. The new variant's doc comment names the lifecycle condition that produces it. Test-hygiene fixes (comments #80-4, #80-5, #80-10) ================================================== - `drop(tokio::spawn(fut))` at two test sites (server/mod.rs lines 1310 and 1956) replaced with `drop(fut)`. Spawning + detaching the JoinHandle left the announcer task alive for the rest of the test binary's lifetime, emitting multicast that could confuse sibling tests bound to the same group. The test only cares that construction returned Ok, so don't poll the future at all. - `announcement_loop_sends_offer_service_when_driven` strengthened from "message_id == SD" to parsing the SD header, filtering for an `OfferService` entry matching the server's configured service_id / instance_id, and asserting major_version plus a non-zero TTL. Matches the pattern used by the sd_state.rs multicast tests (dedicated service_id + recv loop filter) so parallel test traffic doesn't cause false matches. Docs (#80-3, #80-7, #80-8) ========================== - src/lib.rs: the phase-6 quickstart docs claimed the client run-loop future "can be spawned on any executor", but it depends on tokio (select!, time, sockets). Tightened to "spawn on the tokio runtime" plus a one-line rationale. - examples/client_server/src/main.rs and examples/discovery_client/ src/main.rs: both still destructured the old 2-tuple from `Client::new`. Updated to destructure the 3-tuple and spawn the run future. They were already failing `cargo check --workspace`. - README.md: same fix, with explicit wording that not spawning the run future produces `Error::Shutdown` on subsequent client calls. Verification: `cargo test --all-features --lib` 426/427 pass. The one failure (`announcement_loop_sends_offer_service_when_driven`) is a pre-existing multicast-loopback environmental issue on this machine — reproduces on the 76ee480 base without any of these changes. CI will validate it there. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Round-2 Copilot feedback on PR #76 caught 5 resolvable issues (6th rejected — see below): - src/client/inner.rs: the three `push_front(...).ok()` re-enqueue sites (SetInterface x2, SendSD, Subscribe) silently dropped the returned Err. Swapped each to `if let Err(rejected) = ... { ... rejected.reject_with_capacity("request_queue"); }`, matching the primary `push_back` overflow arm. These branches are defensive — by construction the slot we just popped is free — but a future refactor that changes queue usage would otherwise reintroduce the silent-drop-of-oneshot-senders regression cb1d0d1 was specifically written to prevent. - src/server/subscription_manager.rs: `list.push(...).ok()` on a freshly-allocated `SubscribersList` (first subscriber in a new event group) swapped to `.expect(...)` with a message naming the `SUBSCRIBERS_PER_GROUP >= 1` invariant. Tripwires a future cap-0 regression at test time instead of silently losing the only subscriber. - src/client/session.rs: reword the `SessionTracker` doc comment's reference to non-existent "module docs" — point directly at the `SESSION_CAP` constant instead. Rejected: src/client/error.rs `Capacity` variant as a breaking exhaustive-match break. This is consistent with the earlier decision recorded on #75/#80 to accept the breaking change rather than carry deprecated shims or wrap everything in `Error::Io` — the current release is a breaking version bump by design, and hiding the variant in `Io` would lose the lowercase-snake_case tag semantics the new error was specifically designed to carry. No production behavior changes — just defensive tripwires, docs. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Addresses 8 Copilot review comments on PR #80. No structural changes; purely hygiene + doc accuracy. must_use on tokio::spawn(run[_fut]) — `tokio::spawn` returns a `#[must_use]` `JoinHandle`, so bare-statement calls trip the lint. Bound the handle to `let _ = ...` in tests (where we don't want to keep the handle alive) and `let _run_task`/`let _run_handle = ...` in examples and the crate-level doctest (reviewer-suggested wording). Grep confirmed the reviewer's "pattern repeats throughout" hint: 20 sites in src/client/mod.rs tests, 22 in src/client/inner.rs tests, 12 in tests/client_server.rs, plus the 4 explicitly-cited singletons (src/lib.rs doctest, examples/discovery_client, examples/client_server, the one inner.rs site named by line number). All 58 sites fixed. src/server/mod.rs comment cleanup — the announcement_loop test's comment block said "Spawn and immediately drop the handle" followed by "Do NOT spawn", which is self-contradictory since the earlier refactor switched from `drop(tokio::spawn(fut))` to plain `drop(fut)`. Rewrote to describe the actual behavior: we construct the future, assert Ok, and drop it without polling, because spawning would leave the announcer emitting multicast for the remainder of the test binary. README.md correction — the Client quick-start claimed control-channel calls return `Error::Shutdown` if the run-loop future isn't spawned. That is inaccurate: if the future exists but is never polled, the control channel's sender succeeds and the caller hangs forever on the oneshot response. `Shutdown` is only produced once the run-loop future has been dropped or its task cancelled. Rewrote the passage to state that the future must be actively driven and that hangs (not `Shutdown`) are what happens when it isn't. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- client/mod.rs: fix Client::new doc — now returns the run-loop future, does not spawn. - client/inner.rs: apply backpressure on control_receiver.recv() via a select! guard so a full request_queue stalls senders instead of dropping the ControlMessage (which canceled embedded oneshot senders and surfaced as Error::Shutdown, conflating overload with shutdown). - server/mod.rs: rewrite the internally-contradictory "spawn + drop" test comment to match actual behavior (build + drop without polling). - client/inner.rs: rename test_inner_spawn_and_shutdown → test_inner_build_and_shutdown to match the Inner::build API. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Round-2 Copilot feedback on PR #76 caught 5 resolvable issues (6th rejected — see below): - src/client/inner.rs: the three `push_front(...).ok()` re-enqueue sites (SetInterface x2, SendSD, Subscribe) silently dropped the returned Err. Swapped each to `if let Err(rejected) = ... { ... rejected.reject_with_capacity("request_queue"); }`, matching the primary `push_back` overflow arm. These branches are defensive — by construction the slot we just popped is free — but a future refactor that changes queue usage would otherwise reintroduce the silent-drop-of-oneshot-senders regression cb1d0d1 was specifically written to prevent. - src/server/subscription_manager.rs: `list.push(...).ok()` on a freshly-allocated `SubscribersList` (first subscriber in a new event group) swapped to `.expect(...)` with a message naming the `SUBSCRIBERS_PER_GROUP >= 1` invariant. Tripwires a future cap-0 regression at test time instead of silently losing the only subscriber. - src/client/session.rs: reword the `SessionTracker` doc comment's reference to non-existent "module docs" — point directly at the `SESSION_CAP` constant instead. Rejected: src/client/error.rs `Capacity` variant as a breaking exhaustive-match break. This is consistent with the earlier decision recorded on #75/#80 to accept the breaking change rather than carry deprecated shims or wrap everything in `Error::Io` — the current release is a breaking version bump by design, and hiding the variant in `Io` would lose the lowercase-snake_case tag semantics the new error was specifically designed to carry. No production behavior changes — just defensive tripwires, docs. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

…oopback return type has a breaking change and server::start_announcing's loop signature changed

Addresses the six open comments on PR #80 that 76ee480 did not cover; the three structural decisions (Client::new breaking signature, start_announcing -> announcement_loop rename, panic -> typed error) were all confirmed as "break, no wrapper" by the maintainer. Typed Error::Shutdown (comment #80-6) ===================================== Added `Error::Shutdown` to `client::Error` — the variant the run-loop control channel surfaces when its receiver is gone (run future dropped, cancelled, or exited). Converted every public Client method that routes through `control_sender` to return `Err(Error::Shutdown)` on channel closure instead of panicking on `.unwrap()` of the send or recv result: - 9 call sites in client/mod.rs switched from self.control_sender.send(..).await.unwrap(); response.await.unwrap() to self.control_sender.send(..).await.map_err(|_| Error::Shutdown)?; response.await.map_err(|_| Error::Shutdown)? - `request()`'s `response_rx.await.expect(...)` follows the same pattern. - `PendingResponse::response()`'s `.expect(...)` does too. Rewrote the two `#[should_panic]` regression tests that were locking in the panic behavior: - `dropping_run_future_without_spawn_returns_shutdown_error` - `cancelling_run_future_closes_control_channel_returns_shutdown_error` Both now assert `matches!(err, Error::Shutdown)` after the expected drop/abort, so any regression that reintroduces the panic path fails the test on the panic itself and any regression that reverts the typed error fails on the matches! check. Removed the `# Panics` doc sections from affected methods; `set_interface` keeps a `# Panics` note scoped to the interface-lock-poisoning case, which is unrelated to the control channel. The new variant's doc comment names the lifecycle condition that produces it. Test-hygiene fixes (comments #80-4, #80-5, #80-10) ================================================== - `drop(tokio::spawn(fut))` at two test sites (server/mod.rs lines 1310 and 1956) replaced with `drop(fut)`. Spawning + detaching the JoinHandle left the announcer task alive for the rest of the test binary's lifetime, emitting multicast that could confuse sibling tests bound to the same group. The test only cares that construction returned Ok, so don't poll the future at all. - `announcement_loop_sends_offer_service_when_driven` strengthened from "message_id == SD" to parsing the SD header, filtering for an `OfferService` entry matching the server's configured service_id / instance_id, and asserting major_version plus a non-zero TTL. Matches the pattern used by the sd_state.rs multicast tests (dedicated service_id + recv loop filter) so parallel test traffic doesn't cause false matches. Docs (#80-3, #80-7, #80-8) ========================== - src/lib.rs: the phase-6 quickstart docs claimed the client run-loop future "can be spawned on any executor", but it depends on tokio (select!, time, sockets). Tightened to "spawn on the tokio runtime" plus a one-line rationale. - examples/client_server/src/main.rs and examples/discovery_client/ src/main.rs: both still destructured the old 2-tuple from `Client::new`. Updated to destructure the 3-tuple and spawn the run future. They were already failing `cargo check --workspace`. - README.md: same fix, with explicit wording that not spawning the run future produces `Error::Shutdown` on subsequent client calls. Verification: `cargo test --all-features --lib` 426/427 pass. The one failure (`announcement_loop_sends_offer_service_when_driven`) is a pre-existing multicast-loopback environmental issue on this machine — reproduces on the 76ee480 base without any of these changes. CI will validate it there. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Addresses 8 Copilot review comments on PR #80. No structural changes; purely hygiene + doc accuracy. must_use on tokio::spawn(run[_fut]) — `tokio::spawn` returns a `#[must_use]` `JoinHandle`, so bare-statement calls trip the lint. Bound the handle to `let _ = ...` in tests (where we don't want to keep the handle alive) and `let _run_task`/`let _run_handle = ...` in examples and the crate-level doctest (reviewer-suggested wording). Grep confirmed the reviewer's "pattern repeats throughout" hint: 20 sites in src/client/mod.rs tests, 22 in src/client/inner.rs tests, 12 in tests/client_server.rs, plus the 4 explicitly-cited singletons (src/lib.rs doctest, examples/discovery_client, examples/client_server, the one inner.rs site named by line number). All 58 sites fixed. src/server/mod.rs comment cleanup — the announcement_loop test's comment block said "Spawn and immediately drop the handle" followed by "Do NOT spawn", which is self-contradictory since the earlier refactor switched from `drop(tokio::spawn(fut))` to plain `drop(fut)`. Rewrote to describe the actual behavior: we construct the future, assert Ok, and drop it without polling, because spawning would leave the announcer emitting multicast for the remainder of the test binary. README.md correction — the Client quick-start claimed control-channel calls return `Error::Shutdown` if the run-loop future isn't spawned. That is inaccurate: if the future exists but is never polled, the control channel's sender succeeds and the caller hangs forever on the oneshot response. `Shutdown` is only produced once the run-loop future has been dropped or its task cancelled. Rewrote the passage to state that the future must be actively driven and that hangs (not `Shutdown`) are what happens when it isn't. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Fix 64 clippy warnings introduced by this branch's own commits: - let_underscore_future (54): drop the 'let _ =' prefix on 'tokio::spawn(run_fut)' call sites across client/mod.rs, client/inner.rs tests, and tests/client_server.rs — JoinHandle is not #[must_use], so bare expression statements are fine. - new_ret_no_self on Inner::new: rename to Inner::build, since after the spawn-hoist the method returns (sender, receiver, future) rather than a constructed Self. Internal API only (Inner is pub(super)). - manual_async_fn on Inner::run_future: rewrite as async fn; captured state is already Send + 'static so the inferred future keeps the bounds the caller relies on. - double_must_use on Client::{new, new_with_loopback}: swap the bare #[must_use] for a message pointing out that the returned future in the tuple must be spawned. - items_after_statements in announcement_loop test: hoist SID/IID consts to the top of the fn.

- client/mod.rs: fix Client::new doc — now returns the run-loop future, does not spawn. - client/inner.rs: apply backpressure on control_receiver.recv() via a select! guard so a full request_queue stalls senders instead of dropping the ControlMessage (which canceled embedded oneshot senders and surfaced as Error::Shutdown, conflating overload with shutdown). - server/mod.rs: rewrite the internally-contradictory "spawn + drop" test comment to match actual behavior (build + drop without polling). - client/inner.rs: rename test_inner_spawn_and_shutdown → test_inner_build_and_shutdown to match the Inner::build API. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…future `let _ = tokio::spawn(run);` trips clippy::let_underscore_future because `JoinHandle: Future`. Match the pattern already used in examples/discovery_client and the lib.rs doctest. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Mirror the Client::new / Client::new_with_loopback pattern — the returned future must be spawned or awaited, and silently dropping it disables announcements. A `#[must_use = "..."]` attribute nudges callers who forget (e.g. `server.announcement_loop()?;` that discards the inner future). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

When pending_responses is full, the inner loop previously dropped the response oneshot sender, which surfaced to callers as RecvError → Error::Shutdown — misattributing overload as a lifecycle failure. Recover the oneshot sender from the failed insert and send an explicit Err(Error::Capacity("pending_responses")) through it instead. The UDP send has already succeeded at this point; the peer's reply (if any) still arrives on ClientUpdates::Unicast, it just can't be routed back to this specific caller's oneshot. Reserving Error::Shutdown for actual run-loop exit keeps RecvError at PendingResponse::response unambiguous: a cancelled oneshot now only means the run-loop future is gone, not that the map was full. Update the # Errors sections on PendingResponse::response and Client::request to document both the new Capacity("pending_responses") case and the narrower Shutdown contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

PR #80 introduced Error::Capacity("pending_responses") in inner.rs but the tag enumeration in client::Error::Capacity's docstring still listed only "unicast_sockets" and "udp_buffer". Add the new tag and format the list as bullets for readability. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Bind all bare `tokio::spawn(run_fut)` calls in tests and doctests to `let _ = ...` so the `#[must_use]` JoinHandle is explicitly discarded rather than silently dropped. Also fixes the README server quick-start. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Replace `let _ = tokio::spawn(...)` with stored handles in README and the client_server example so panics or unexpected task exits don't go silently unobserved. README server quick-start uses tokio::select! to surface either loop exiting; example stores the handle so the task lives for the duration of main. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

- Inner::build and server/README.md: replace "executor" with "Tokio runtime" to avoid implying runtime-agnostic execution - announcement_loop_sends_offer_service_when_driven: change SID/IID from 0x005C/0x0001 (used throughout the module) to 0xAA01/0xFF01 so the "not used elsewhere" comment is actually true Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…I ref Add missing error type to Result<T> entries and replace "their executor" with "the Tokio runtime". Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Final state of #81, squashed from 8 commits to keep the rebase onto the rewritten #80 tractable. The intermediate history includes a transient select_biased! step that was reverted to plain select! for fairness within the same PR, which would have produced a contradictory mid-rebase state if replayed individually. Original commits, oldest first: - 888bcfa Add futures dep behind client/server features for upcoming phase 7 work. Enables futures::FutureExt / pin_mut / select macros in the event loops; no code paths use it yet. - be42199 Dropped tokio::select in favor of futures::select_biased! (with FutureExt::fuse + pin_mut!) in socket_manager and client::inner; removed tokio::spawn(...) from Client::new (the run-future is now returned to the caller); migrated client.start_sd_announcements -> sd_announcements_loop returning a future for the caller to spawn. - 2f90058 Utilize TokioTimer for the loop sleeps, one step closer to a bare-metal replacement. - 0df42f2 Address adversarial review for phase 7 — fairness, readability, coverage: * select_biased! -> select! at the 3 event-loop sites (socket_manager, inner::run_future, server::run). Restores random per-poll fairness and eliminates the arm-starvation risk introduced by biased ordering. * Imports Timer + TokioTimer at each Timer call site so UFCS reduces to method syntax (TokioTimer.sleep(d)). * Hoists socket_manager's Outcome<P> enum out of spawn_socket_loop's async block to module scope. * Updates stale "phase 6" doc references to point at the actual planned hoist phase (8, alongside the bare-metal example). * New tests: sd_announcements_loop_cadence_stays_close_to_requested bind_with_transport_carries_traffic_end_to_end bind_with_transport_propagates_factory_error client_new_run_future_is_send_static - 27f9e67 phase 7: respond to PR #81 feedback (Copilot): (1) correct futures dep comment in Cargo.toml to describe actual usage (select! + FutureExt::fuse / pin_mut!); (2) rename recv error log to "Transport recv failed" with binding `recv_err` since the error is from socket.recv_from, not a parse step; (3) drop the pre-loop sleep in sd_announcements_loop so the first announcement lands at ~1x interval instead of ~2x; in-loop sleep carries the initial-delay role. New test sd_announcements_loop_first_emit_within_one_interval pins first-emit latency below 250ms at 100ms interval. - 4f7f9d5 docs: fix stale Client API name in passive-server error message (start_sd_announcements -> sd_announcements_loop). - f5c674a chore(clippy): tidy new warnings in phase7 PR: * match_wildcard_for_single_variants on SocketAddr match — make the wildcard explicit as `other @ SocketAddr::V6(_)`. * manual_async_fn on AlwaysBusyFactory test-impl of TransportFactory::bind: rewrite as async fn. - 33ce551 round-3: fix #[ignore] reasons on sd_announcements_loop tests. Align both tests under #[ignore] with an accurate reason referencing the real constraint (MULTICAST on the loopback interface); drop the stale sd_state.rs reference. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-27T13:23:21Z

+    /// let (client, mut updates, run) = Client::<RawPayload>::new(Ipv4Addr::LOCALHOST);
+    /// let _run_task = tokio::spawn(run);
+    /// // ...interact with `client` and `updates`...
+    /// # let _ = (client, updates);
+    /// # }
+    /// ```
+    #[must_use = "the returned run-loop future must be spawned (e.g. tokio::spawn) for the client to make progress"]
+    pub fn new(
+        interface: Ipv4Addr,
+    ) -> (
+        Self,
+        ClientUpdates<MessageDefinitions>,
+        impl core::future::Future<Output = ()> + Send + 'static,
+    ) {
        Self::new_with_loopback(interface, false)


The #[must_use = "..."] on Client::new won’t warn if a caller destructures the tuple and then accidentally drops/ignores only the run_future (since the overall return value is still “used”). If the goal is to make forgetting to drive the run loop harder, consider returning a small ClientDriver/RunLoop newtype marked #[must_use] (or a struct with a #[must_use] driver field) instead of an unstructured 3-tuple.

Suggested change

/// let (client, mut updates, run) = Client::<RawPayload>::new(Ipv4Addr::LOCALHOST);

/// let _run_task = tokio::spawn(run);

/// // ...interact with `client` and `updates`...

/// # let _ = (client, updates);

/// # }

/// ```

#[must_use = "the returned run-loop future must be spawned (e.g. tokio::spawn) for the client to make progress"]

pub fn new(

interface: Ipv4Addr,

) -> (

Self,

ClientUpdates<MessageDefinitions>,

impl core::future::Future<Output = ()> + Send + 'static,

) {

Self::new_with_loopback(interface, false)

/// let simple_someip::client::ClientParts { client, mut updates, run } =

/// Client::<RawPayload>::new(Ipv4Addr::LOCALHOST);

/// let _run_task = tokio::spawn(run);

/// // ...interact with `client` and `updates`...

/// # let _ = (client, updates);

/// # }

/// ```

#[must_use = "the returned run-loop future must be spawned (e.g. tokio::spawn) for the client to make progress"]

pub struct ClientParts<P, R> {

pub client: Client<P>,

pub updates: ClientUpdates<P>,

#[must_use = "the returned run-loop future must be spawned (e.g. tokio::spawn) for the client to make progress"]

pub run: R,

}

pub fn new(

interface: Ipv4Addr,

) -> ClientParts<

MessageDefinitions,

impl core::future::Future<Output = ()> + Send + 'static,

> {

let (client, updates, run) = Self::new_with_loopback(interface, false);

ClientParts {

client,

updates,

run,

}

Copilot · 2026-04-27T13:23:22Z

+        let announce_fut = server
+            .announcement_loop()
+            .expect("announcement_loop should build on a non-passive server");
+        tokio::spawn(announce_fut);


This test now drives announcements via announcement_loop(), but the surrounding test name/doc/expect strings still reference start_announcing, which makes grepping and future maintenance confusing. Consider renaming the test (and any associated messages) to reference announcement_loop instead, to match the public API after the rename.

Copilot · 2026-04-27T13:23:22Z

                    // Receive a discovery message
                    discovery = Inner::receive_discovery(discovery_socket) => {
-                        trace!("Received discovery message: {:?}", discovery);
-                        match discovery {
-                            Ok((source, someip_header, sd_header)) => {
-                                // Extract session ID from SOME/IP request_id (lower 16 bits)
-                                let session_id = (someip_header.request_id() & 0xFFFF) as u16;
-                                let sd_payload = PayloadDefinitions::new_sd_payload(&sd_header);
-                                // Extract reboot flag from the SD payload flags
-                                let reboot_flag = sd_payload
-                                    .sd_flags()
-                                    .map_or(crate::protocol::sd::RebootFlag::Continuous, |f| {
-                                        f.reboot()
-                                    });
-
-                                // Track sender session/reboot state for every SD entry
-                                // that identifies a service instance, not only
-                                // offer/stop-offer entries. This ensures reboot
-                                // detection works for all SD traffic (FindService,
-                                // Subscribe, SubscribeAck, etc.).
-                                let mut rebooted = false;
-                                for (svc_id, inst_id) in sd_payload.service_instances() {
-                                    let verdict = session_tracker.check(
-                                        source,
-                                        TransportKind::Multicast,
-                                        svc_id,
-                                        inst_id,
-                                        session_id,
-                                        reboot_flag,
-                                    );
-                                    if verdict == SessionVerdict::Reboot {
-                                        rebooted = true;
-                                    }
+                    trace!("Received discovery message: {:?}", discovery);
+                    match discovery {
+                        Ok((source, someip_header, sd_header)) => {


cargo fmt --check is enforced in CI, and this select! arm body appears to have lost rustfmt indentation (the body starts at the same indent level as the arm label). Please re-run rustfmt (or manually fix indentation) so the discovery = ... => { ... } block is formatted correctly; the subsequent unicast = ... arm also looks mis-indented.

JustinKovacich · 2026-06-09T20:56:07Z

Closing without merge to declutter the stack: this phase's changes are carried in full by the consolidated lineage under PR #114 (phase 21), which the next development stack builds on. Branch is retained.

JustinKovacich requested a review from Copilot April 23, 2026 00:28

Copilot started reviewing on behalf of JustinKovacich April 23, 2026 00:28 View session

Copilot AI reviewed Apr 23, 2026

View reviewed changes

JustinKovacich force-pushed the feature/hoist_tokio_spawn branch from be42199 to 76ee480 Compare April 23, 2026 01:46

This was referenced Apr 23, 2026

Select biased timer #81

Closed

Phase8 bare metal #82

Closed

JustinKovacich self-assigned this Apr 23, 2026

JustinKovacich added documentation Improvements or additions to documentation enhancement New feature or request labels Apr 23, 2026

JustinKovacich mentioned this pull request Apr 23, 2026

Add a tokio transport and socket manager #79

Closed

JustinKovacich force-pushed the feature/hoist_tokio_spawn branch from 2c55c8a to d0043ad Compare April 23, 2026 17:20

JustinKovacich requested a review from Copilot April 23, 2026 17:41

Copilot started reviewing on behalf of JustinKovacich April 23, 2026 17:41 View session

Copilot AI reviewed Apr 23, 2026

View reviewed changes

JustinKovacich force-pushed the feature/hoist_tokio_spawn branch from d0043ad to 8b2ff9a Compare April 23, 2026 17:58

JustinKovacich requested a review from Copilot April 24, 2026 20:36

Copilot started reviewing on behalf of JustinKovacich April 24, 2026 20:36 View session

Copilot AI reviewed Apr 24, 2026

View reviewed changes

Comment thread src/client/mod.rs Outdated

Comment thread src/client/inner.rs Outdated

Comment thread src/server/mod.rs Outdated

Comment thread src/client/inner.rs Outdated

JustinKovacich requested a review from Copilot April 24, 2026 21:09

Copilot started reviewing on behalf of JustinKovacich April 24, 2026 21:10 View session

Copilot AI reviewed Apr 24, 2026

View reviewed changes

Comment thread examples/client_server/src/main.rs Outdated

JustinKovacich requested a review from Copilot April 24, 2026 22:02

JustinKovacich requested a review from Copilot April 25, 2026 00:04

Copilot started reviewing on behalf of JustinKovacich April 25, 2026 00:04 View session

Copilot AI reviewed Apr 25, 2026

View reviewed changes

Comment thread src/server/README.md Outdated

JustinKovacich requested a review from Copilot April 25, 2026 00:15

Copilot started reviewing on behalf of JustinKovacich April 25, 2026 00:15 View session

Copilot AI reviewed Apr 25, 2026

View reviewed changes

JustinKovacich marked this pull request as ready for review April 25, 2026 00:19

JustinKovacich force-pushed the feature/tokio_transport_and_socket_manager branch from 0109693 to 92cf168 Compare April 25, 2026 00:43

JustinKovacich and others added 14 commits April 24, 2026 20:44

Apply hoists: run_future doesnt call tokio::spawn, client::new/with_l…

94cb236

…oopback return type has a breaking change and server::start_announcing's loop signature changed

Add testing, add sensible panics, updated docs

07f7abe

docs(server): fix Result signatures and executor wording in README AP…

82df573

…I ref Add missing error type to Result<T> entries and replace "their executor" with "the Tokio runtime". Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

JustinKovacich force-pushed the feature/hoist_tokio_spawn branch from c162474 to 82df573 Compare April 25, 2026 00:48

JustinKovacich requested a review from Copilot April 27, 2026 13:16

Copilot started reviewing on behalf of JustinKovacich April 27, 2026 13:16 View session

Copilot AI reviewed Apr 27, 2026

View reviewed changes

JustinKovacich closed this Jun 9, 2026

Conversation

JustinKovacich commented Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

JustinKovacich commented Jun 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JustinKovacich commented Apr 23, 2026 •

edited

Loading