Add the spawner trait#83
Closed
JustinKovacich wants to merge 2 commits into
Closed
Conversation
Closed
JustinKovacich
added
documentation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces an executor-agnostic Spawner trait and wires it into the client’s per-socket I/O loop submission points, moving the crate closer to bare-metal portability while keeping the existing std + tokio behavior via a TokioSpawner adapter.
Changes:
- Added
transport::Spawneras a minimal task-submission abstraction for driving per-socket I/O loops concurrently with the client run loop. - Implemented
TokioSpawner(wrapper overtokio::spawn) and plumbed a user-providedSpawnerthroughClient/InnerandSocketManagerbind paths. - Updated docs/examples and added tests to verify spawns are routed through the injected
Spawner.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/transport.rs | Adds the new Spawner trait and extensive rationale/usage docs. |
| src/tokio_transport.rs | Introduces TokioSpawner and implements transport::Spawner using tokio::spawn. |
| src/lib.rs | Re-exports Spawner and TokioSpawner; updates crate docs on bare-metal limitations. |
| src/client/socket_manager.rs | Replaces direct tokio::spawn with a caller-supplied Spawner for socket loops. |
| src/client/mod.rs | Adds Client::new_with_spawner_and_loopback constructor and a routing test. |
| src/client/inner.rs | Stores the injected spawner in Inner and routes discovery/unicast binds through it; adds tests. |
| examples/discovery_client/src/main.rs | Minor formatting update to match constructor style. |
| examples/client_server/src/main.rs | Minor formatting update to match constructor style. |
| examples/bare_metal/src/main.rs | Updates caveats and demonstrates compiling against the new Spawner trait. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
JustinKovacich
added a commit
that referenced
this pull request
Apr 23, 2026
Round-2 Copilot feedback on PR #79: - src/tokio_transport.rs:129 recv_from: `tokio::net::UdpSocket:: recv_from` silently truncates when the caller's buf is smaller than the datagram — it does not expose a truncation flag. The `ReceivedDatagram::truncated` field therefore always reports `false` on the Tokio backend. Reliable truncation detection would require a libc + unsafe `recvmsg`/MSG_TRUNC path, deferred to the phase 10+ bare-metal refactor. Added a precise in-line comment naming the platform limitation so callers don't mis-trust the field. - src/tokio_transport.rs:213 map_io_error: the post-mapping logging previously unconditionally used `warn!` for every I/O error, which turned common steady-state conditions (TimedOut, Interrupted, ConnectionRefused during transient outages) into warning noise that can drown out actionable signal. Triaged by kind: common-path kinds drop to `debug!`; unexpected / misconfiguration-indicating kinds (PermissionDenied, AddrInUse, NetworkUnreachable, fallback Other) stay at `warn!`. Comment explains the rationale. - src/transport.rs module docs: the `crate::tokio_transport::*` intra-doc links broke under default-feature rustdoc builds (the `tokio_transport` module is gated to `client`/`server`). Same resolution as the PR #83 intra-doc fixes in ee305e0: render the paths as code literals and add an inline note that the module requires those features. Keeps the information, drops the link. No new behavior paths introduced — logging changes + docs only. Existing tokio_transport test suite (6/6) still passes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
JustinKovacich
added a commit
that referenced
this pull request
Apr 23, 2026
Follow-up to 3f93900 — the intra-doc fix for the `crate::tokio_transport::*` links in transport.rs didn't get saved before the commit closed. Same resolution as the PR #83 fix in ee305e0: render feature-gated paths as code literals rather than intra-doc links, add inline note explaining why. Verified with `RUSTDOCFLAGS=-D rustdoc::broken-intra-doc-links cargo doc --no-deps` — default-feature docs now clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 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.
JustinKovacich
added a commit
that referenced
this pull request
Apr 25, 2026
Round-2 Copilot feedback on PR #79: - src/tokio_transport.rs:129 recv_from: `tokio::net::UdpSocket:: recv_from` silently truncates when the caller's buf is smaller than the datagram — it does not expose a truncation flag. The `ReceivedDatagram::truncated` field therefore always reports `false` on the Tokio backend. Reliable truncation detection would require a libc + unsafe `recvmsg`/MSG_TRUNC path, deferred to the phase 10+ bare-metal refactor. Added a precise in-line comment naming the platform limitation so callers don't mis-trust the field. - src/tokio_transport.rs:213 map_io_error: the post-mapping logging previously unconditionally used `warn!` for every I/O error, which turned common steady-state conditions (TimedOut, Interrupted, ConnectionRefused during transient outages) into warning noise that can drown out actionable signal. Triaged by kind: common-path kinds drop to `debug!`; unexpected / misconfiguration-indicating kinds (PermissionDenied, AddrInUse, NetworkUnreachable, fallback Other) stay at `warn!`. Comment explains the rationale. - src/transport.rs module docs: the `crate::tokio_transport::*` intra-doc links broke under default-feature rustdoc builds (the `tokio_transport` module is gated to `client`/`server`). Same resolution as the PR #83 intra-doc fixes in ee305e0: render the paths as code literals and add an inline note that the module requires those features. Keeps the information, drops the link. No new behavior paths introduced — logging changes + docs only. Existing tokio_transport test suite (6/6) still passes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
JustinKovacich
added a commit
that referenced
this pull request
Apr 25, 2026
Follow-up to 3f93900 — the intra-doc fix for the `crate::tokio_transport::*` links in transport.rs didn't get saved before the commit closed. Same resolution as the PR #83 fix in ee305e0: render feature-gated paths as code literals rather than intra-doc links, add inline note explaining why. Verified with `RUSTDOCFLAGS=-D rustdoc::broken-intra-doc-links cargo doc --no-deps` — default-feature docs now clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
JustinKovacich
force-pushed
the
feature/phase8_bare_metal
branch
from
1e92f43 to
e4ab414
Compare
Final state of #83, squashed from 6 commits to keep the rebase onto the rewritten #82 tractable. Original commits, oldest first: - cfbd17a Added a Spawner trait in transport.rs. TokioSpawner is the default std impl. Client::new_with_spawner_and_loopback accepts a caller-supplied spawner so the bare-metal port can drive socket loops without tokio. New unit tests and bare_metal example now demonstrates usage with a MockSpawner. - 9122afd New bind tests; corrected documentation throughout to reflect the concrete next steps toward bare-metal support. - ee305e0 phase 9: fix intra-doc links that break default-feature rustdoc builds. Three sites that referenced feature-gated items via intra-doc links: tokio_transport's `[Spawner]` -> `[crate::transport::Spawner]`; transport.rs link to `[crate::tokio_transport::TokioSpawner]` -> code literal with feature-gate note; same pattern at transport.rs:478 for `[crate::client::Client]`. Also lib.rs module table: intra-doc links for feature-gated `[client]` / `[server]` modules -> plain code literals. - d618081 round-2: fix intra-doc link + correct channel-capacity docs. Replace the intra-doc link to Client::new_with_spawner_and_loopback in tokio_transport.rs with a plain code literal (breaks default-feature rustdoc in feature="server"-without-feature="client"). Correct socket_manager module doc: per-socket mpsc channels are 16/16 for discovery sockets, 4/4 for unicast (verified against mpsc::channel call sites). - d2c44e0 chore(clippy): address new warnings in phase9 PR (13 fixes). doc_markdown: backtick `no_alloc`, `no_std`, `bare_metal` identifiers in new docs. double_must_use on Client::new_with_spawner_and_loopback: replace bare #[must_use] with a message about the run-loop future. dead_code on SocketManager::{bind, bind_discovery_seeded}: gate under #[cfg(test)] since the Spawner refactor removes non-test callers. - 721223c round-3: address Copilot review on socket_manager doc links + test JoinHandle. socket_manager: render TokioTransport / TokioSpawner refs as code literals (same rustdoc-feature-gating pattern as elsewhere in the PR) so default-feature doc builds no longer see broken intra-doc links. client::tests: bind the JoinHandle from tokio::spawn(future) to `_` in the CountingSpawner test so the #[must_use] lint does not fire. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
JustinKovacich
force-pushed
the
feature/phase9_spawner_trait
branch
from
721223c to
e87c128
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+91
to
+93
| /// Zero-size unit struct; every `Inner<P, TokioSpawner>` / `Client<P, | ||
| /// TokioSpawner>` pays nothing for the abstraction. Bare-metal | ||
| /// consumers substitute their own `Spawner` via the |
There was a problem hiding this comment.
The doc comment claims Client<P, TokioSpawner> exists, but Client is only generic over the payload type (the spawner is stored in Inner, not exposed as a second Client type parameter). This is misleading for readers and for rustdoc search. Suggest updating the wording to reference Client<P> (or just Client) and, if desired, keep the Inner<P, TokioSpawner> example for the internal type that actually carries the spawner.
Suggested change
| /// Zero-size unit struct; every `Inner<P, TokioSpawner>` / `Client<P, | |
| /// TokioSpawner>` pays nothing for the abstraction. Bare-metal | |
| /// consumers substitute their own `Spawner` via the | |
| /// Zero-size unit struct; every `Inner<P, TokioSpawner>` / `Client<P>` | |
| /// pays nothing for the abstraction. Bare-metal consumers substitute | |
| /// their own `Spawner` via the |
Adversarial review (4 parallel reviewers + 87 Copilot comments triaged
to 15 unresolved threads) surfaced 86 ranked items: 1 blocker, 13 high,
19 medium, 25 low, 28 nits. This round addresses everything fixable
without an architectural redesign (4 explicitly deferred to phases 10+:
ClientParts struct refactor, fully injectable Server::Timer, TTL
reaping, and a tokio-feature opt-out path).
Highlights, by phase:
A. Blocker — bare_metal canary
examples/bare_metal/src/main.rs's MockSocket signatures
contradicted the phase-4 &self migration (4 x E0053). Fixed all
four impl receivers, replaced MockSpawner-that-drops-the-future
with a WorkingSpawner that actually polls (the trait contract
requires polling, and the canary previously demonstrated the wrong
pattern). cargo build -p bare_metal + cargo run -p bare_metal both
pass; assert!ed that the spawned future was polled to completion.
B. Semantic correctness
- Client::reboot_flag now returns Result<RebootFlag, Error>;
QueryRebootFlag carries Result<_, Error>;
force_sd_session_wrapped_for_test parallel migration. Matches
every other public Client method's Shutdown semantics
(regression test added).
- SocketManager::send no longer .expect-panics on a dropped
response oneshot. Phase-9 user-supplied Spawner made that path
reachable.
- Pre-encode UDP_BUFFER_SIZE check on send path mirrors the
EventPublisher's existing guard; oversize messages now return
Error::Capacity(\"udp_buffer\") uniformly.
- request_queue overflow notifies senders with Capacity instead of
dropping (callers see typed Capacity, not Shutdown via RecvError).
- SocketManager recv-error hot loop bounded at 16 consecutive
failures.
- server::EventPublisher::publish_event returns Err(Error::E2e(_))
on protect failure instead of silently sending the UNPROTECTED
payload.
- SD Subscribe with mismatched major_version now NACKs.
- SdStateManager exposes next_session_id_with_reboot_flag()
atomic pair; eliminates a TOCTOU race around the wrap boundary.
C. Docs / CHANGELOG / intra-doc links
- CHANGELOG [Unreleased] now covers every phase 7-9 breaking
change.
- README pins simple-someip = \"0.7\"; transport module + bare_metal
feature row added.
- server/README.md SD diagram corrected (Unicast=true);
register_subscriber Result documented.
- Spawner trait: JoinHandle policy made explicit (fire-and-forget
by design); embassy claim softened.
- ReceivedDatagram::truncated and max_datagram_size MTU wording
corrected.
- All broken intra-doc links demoted to backtick code literals.
cargo doc --all-features and --no-default-features both clean.
- traits.rs: set_reboot_flag default no-op so std-but-not-client
consumers don't have to implement an unused method.
D. Spawn-handle hygiene
62 sites converted from bare or 'let _ = tokio::spawn(x)' to
'let _run_handle = tokio::spawn(x)'. Suppresses
clippy::let_underscore_future under cargo 1.91.
E-G. Drift cleanup, low, nits
- Unique service IDs per integration test via static AtomicU16;
parallel-execution caveat documented (SO_REUSEPORT routing flake
pre-existing on main; --test-threads=1 required).
- clippy --fix swept the bulk; manual cleanup for what remained.
- publish_raw_event size guard moved BEFORE Header::new_event.
- const _: () = assert!(...) for SUBSCRIBERS_PER_GROUP and
EVENT_GROUPS_CAP compile-time invariants.
- SessionTracker / sd_state docs migrated to typed-flag language.
- announcement_loop_emits_first_offer test now aborts spawned
handle.
- select! cancel-safety contract documented in server::run.
- bare_metal block_on doc points at cassette /
embassy_executor::block_on.
Verification:
- cargo check across {all-features, no-default, client-only,
server-only, client+server, bare_metal-feature}: all clean.
- cargo doc {all-features, no-default-features}: zero warnings.
- cargo clippy --workspace --all-features --all-targets -D warnings:
zero warnings.
- cargo test --lib --all-features: 452/452 pass.
- cargo test --test client_server -- --test-threads=1: 11/11 pass.
- cargo run -p bare_metal: end-to-end success.
- cargo fmt --check: clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added a spawner trait to inch closer to bare metal support
This PR is part of a chain. See Prev: #82; See Next: ???