Phase17 cleanup#96
Draft
JustinKovacich wants to merge 16 commits into
Draft
Conversation
- client::socket_manager: when E2E protect() fails for a configured key, return Err(Error::E2e(_)) and continue instead of silently sending the unprotected datagram. A configured key must never leak in the clear. - Server::new_with_deps and Server::new_passive_with_deps: back-fill config.local_port from unicast_socket.local_addr() after bind. Fixes SD offers / event publishers advertising port 0 when the caller passed local_port=0 to let the kernel pick an ephemeral port. - tokio_transport::bind_with_options: apply multicast_loop_v4 when the flag is true OR a multicast interface is configured. Previously the loop flag was silently dropped when multicast_if_v4 was None, even if the caller explicitly asked for loop=true. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ends Both non-tokio channel backends previously violated the OneshotSend / OneshotRecv / MpscSend / MpscRecv / UnboundedSend / UnboundedRecv close contracts in src/transport.rs: - Embassy-Arc backend (src/embassy_channels.rs): all six contracts were broken — OneshotSend always Ok, OneshotRecv literally `Ok(...)` (never Cancelled), MpscSend always Ok, MpscRecv hung forever on all-senders- drop, Unbounded same. A subscriber Client whose ClientUpdate receiver drops would hang the publisher. - Static-pool backend (src/static_channels/mod.rs): partial — recv side was correct, but OneshotSend ignored O_RECEIVER_ALIVE, StaticUnboundedSender::send_now ignored the closed flag, and StaticBoundedSender::send awaited embassy's chan.send() with no race against receiver-drop, so it would deadlock if the channel was full when the receiver disappeared. Fixes: Embassy backend: full rewrite to wrap each Channel in an Inner struct that tracks sender_count, receiver_alive, closed flag, recv_waker, and send_waker. Senders short-circuit on closed; receivers race try_receive against the closed flag with a waker register-then-recheck pattern. Bounded sender pins the embassy SendFuture on the stack and races it against send_waker so receiver-drop wakes pending sends. Static-pool backend: added send_waker to MpscSlot. StaticOneshotSender checks O_RECEIVER_ALIVE before try_send. StaticUnboundedSender::send_now checks closed. StaticBoundedSender::send pins embassy's SendFuture and races against send_waker. Both bounded and unbounded receiver Drops now wake send_waker so blocked senders observe the close. Tests: 7 new embassy unit tests covering close-semantic round-trips on each channel family. 4 new static-channels tests covering sender-side close detection (oneshot fast path, bounded fast path, bounded mid-await unblock, unbounded fast path). Existing tests unchanged. Full suite including the no-alloc witness still green. Multi-sender contention on a closed bounded channel uses a single AtomicWaker per slot — only the most-recent registrant wakes immediately. Other awaiting senders converge on the next poll. This is documented in both backends. Also nudges two earlier multicast-loop / channel-doc comments to American spelling. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous Client::new_with_deps required S: Spawner (Send + 'static spawn) and F::Socket: Send + Sync, blocking embassy-style executors where task state and socket handles are typically !Send. Customers targeting embassy with task-arena = 0 could not construct a Client at all. Introduces: - LocalSpawner trait (src/transport.rs): single-threaded counterpart to Spawner. spawn_local takes `impl Future + 'static` (no Send). Independent of Spawner — an executor MAY implement both (current_thread tokio + LocalSet), only Spawner (multi-thread tokio), or only LocalSpawner (single-task embassy). - BindDispatch trait + SpawnerDispatch / LocalSpawnerDispatch impl structs (src/client/bind_dispatch.rs, crate-private): abstract the bind-and-spawn step. Each impl carries the factory + spawner pair and routes bind requests to the matching SocketManager method. - SocketManager::bind_with_transport_local and bind_discovery_seeded_with_transport_local: parallel to the existing Send variants; relaxed bounds (F::Socket: 'static, S: LocalSpawner) and spawner.spawn_local dispatch. - Inner refactor: generic params drop `<F, S>` and gain `<D>`. The factory + spawner fields are replaced with a single dispatch field of trait BindDispatch. run_future is unchanged — bind_discovery and bind_unicast now call self.dispatch.bind_*. socket_loop_future's Send bounds were relaxed to `'static` so the same body serves both paths; Send-ness is inferred from the dispatch's auto-traits. - Client::new_with_deps_local: !Send constructor that takes a LocalSpawner-bearing ClientDeps and returns `impl Future<Output = ()> + 'static` (no Send). ClientDeps's S: Spawner bound was relaxed; both new_with_deps and new_with_deps_local apply the appropriate trait bound at the constructor call site. Witness test: tests/bare_metal_client.rs adds client_constructible_with_local_spawner — runs Client::new_with_deps_local inside a tokio LocalSet using spawn_local. Pool size for TestStaticChannels bumped from 1→4 so the two parallel-running witness tests don't collide on the process-global static pool. 482 lib tests + 9 bare-metal/static-channels/no-alloc integration tests pass. The 5 client_server UDP-bound tests fail with the same environment errors they show on HEAD (pre-existing). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Server hot path heap-allocated on every event publish, every SD
announcement tick, every FindService response, every Subscribe Ack, and
every Subscribe Nack. None of these allocations were feature-gated, so
bare-metal Server users hit them all. The phase-16 no-alloc claim
covered handle storage and channel pools but explicitly disclaimed
run-loop coverage; this change closes that gap for the visible per-
event paths.
Separately, SubscriptionHandle's RPITIT futures hard-coded `+ Send`,
contradicting the trait surface's "single-threaded executors get the
!Send relaxation" design statement. This blocked embassy-style
SubscriptionHandle implementations.
Changes:
- SubscriptionHandle trait (src/server/subscription_manager.rs):
* Drop `+ Send` from subscribe / unsubscribe / new for_each_subscriber
RPITIT futures, and drop the `Send + Sync` supertrait bounds.
Single-threaded subscription tables (e.g. critical-section
Mutex<RefCell<…>>) can now satisfy the trait.
* Replace `get_subscribers -> Vec<Subscriber>` with
`for_each_subscriber<F: FnMut(&Subscriber)> -> usize`. The visitor
pattern lets callers iterate under the read lock without an owned
snapshot — eliminating the per-publish heap allocation.
- EventPublisher (src/server/event_publisher.rs):
* publish_event / publish_raw_event snapshot subscriber addresses
into `heapless::Vec<SocketAddrV4, 16>` (stack-allocated, sized to
the per-group capacity in subscription_manager) before releasing
the read lock and dispatching async sends. Truncation beyond the
cap is logged but does not silently drop subscribers; the cap
matches the production limit on the underlying table.
* has_subscribers / subscriber_count call for_each_subscriber with a
no-op closure and read the returned count.
- SD encoders (src/server/sd_state.rs and src/server/mod.rs):
* send_offer_service (multicast announcement, fires every 1s),
send_unicast_offer (FindService reply), send_subscribe_ack_from_view,
send_subscribe_nack_from_view: replace the four `Vec::new` +
`extend_from_slice` patterns with a stack `[u8; UDP_BUFFER_SIZE]`
buffer plus `encode_to_slice` for both the SOME/IP header and the
SD payload. No alloc on the per-tick / per-event path.
- Tests:
* tests/bare_metal_server.rs: update MockSubscriptions to implement
the new for_each_subscriber method; drop +Send from the mock's
RPITIT futures.
* tests/bare_metal_client_local.rs (new): the LocalSpawner Client
construction witness moved from bare_metal_client.rs to its own
test binary so it has its own static channel pool. Sharing the
pool across two parallel `#[tokio::test]` cases caused flaky
pool-exhaustion failures because the LocalSet test's spawn_local
drop ordering wasn't tight enough to release slots before the
sibling test claimed them.
* tests/bare_metal_client.rs: pool sizes restored to their original
minimal values; LocalSpawner test removed (lives in the new
sibling file).
* Cargo.toml: register bare_metal_client_local as its own [[test]].
482 lib tests + all bare-metal/static-channels/no-alloc/server-mock
integration tests pass. The 6 client_server UDP-bound tests fail with
the same environment errors they show on HEAD when run in parallel
(they pass individually) — pre-existing flaky behavior, not a
regression.
What customers gain: a Server backed by a SubscriptionHandle on a
critical-section primitive (no Send/Sync, no Vec) is now structurally
expressible. The Server's own `Arc<F::Socket>` / `Arc<EventPublisher>`
fields remain construction-time allocations, which is acceptable per
the no-alloc-after-construction contract.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Each of the 6 mock-using sites (4 tests + 2 examples) had two latent bugs flagged by reviewers: 1. `MockRecvFut::poll` returned `Pending` on an empty inbound queue after calling `cx.waker().wake_by_ref()`. That re-arms the waker immediately, so the run-loop polls in a tight CPU-bound spin — easily a 100% CPU peg in a test environment. 2. `MockTimer::sleep` used `tokio::task::yield_now()` and ignored the `duration` parameter, violating the `Timer` trait's "MAY overshoot but MUST NOT undershoot" contract. Tests that assert on announcement-loop pacing relied on this bug to fire send-tos in tight loops. Fixes: - MockPipe gains an `inbound_waker: Mutex<Option<Waker>>` field plus a `deliver_inbound(bytes, source)` helper that pushes the datagram and wakes the registered receiver. Existing tests that don't drive inbound traffic just stay parked until aborted; future tests can inject ingress through `deliver_inbound` and the receiver actually wakes (no busy-spin, no lost wakeups). - MockRecvFut::poll registers `cx.waker().clone()` on the pipe's waker slot in the empty case and re-checks the queue after registration to close the lost-wakeup window between pop_front and waker.store. No more `wake_by_ref` self-rearm. - MockTimer::sleep delegates to `tokio::time::sleep(duration)`, which honors the trait contract. The test runtime is `#[tokio::test]` anyway (tokio is a dev-dependency); the witness is "the production crate's no-tokio path compiles," not "the test runs without tokio at all." Updated header comments in both example crates to note that `MockTimer` now uses `tokio::time::sleep`. All lib + bare-metal/static-channels/no-alloc/server-mock/local-spawner tests pass. The 5–6 client_server UDP-bound tests still fail with the same environment errors they show on HEAD (not a regression). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Breaking Changes: - SubscriptionHandle: replaced `get_subscribers() -> Vec` with `for_each_subscriber(F)` visitor pattern; removed `+ Send` from RPITIT - Updated bare_metal_server example for new SubscriptionHandle API Documentation: - Remove all "phase-N" references from docs/comments - Align README.md and lib.rs feature tables - Fix 10 broken intra-doc links (embassy_channels, static_channels, transport, server/event_publisher, client/mod) - Update stale refs: static_channels!, panic docs, MockSpawner, paths Features: - Add `embassy_channels` feature to gate `extern crate alloc` separately from `bare_metal`, so static_channels users don't need alloc Tests: - Add tests/bare_metal_e2e.rs: full Client+Server wiring through mock transport with define_static_channels! (2 tests) Code Quality: - Fix error types: TransportError instead of io::Error in unicast_local_addr, socket_addr_v4 - Add #[allow(clippy::single_match_else)] to bare_metal examples - cargo fmt, clippy --pedantic clean
Contributor
There was a problem hiding this comment.
Pull request overview
This PR performs “phase 17” cleanup by updating docs and feature gating, improving bare-metal/no-alloc pathways (including new LocalSpawner support), and refactoring subscription/event publishing APIs to avoid per-event heap allocation.
Changes:
- Introduces
transport::LocalSpawnerandClient::new_with_deps_localplus bind/spawn dispatch plumbing to support single-threaded /!Sendtransports. - Redesigns
SubscriptionHandleto a visitor-basedfor_each_subscriberAPI and updatesEventPublisherto use a stack snapshot buffer (no per-eventVecalloc). - Updates bare-metal and allocator gating (
embassy_channelsfeature) and adds/updates multiple bare-metal witness/E2E tests and examples.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/static_channels_alloc_witness.rs | Updates allocation-witness docs and mock transport recv waking behavior. |
| tests/no_alloc_witness.rs | Formatting/clarity changes in no-alloc panic allocator witness. |
| tests/client_server.rs | Minor doc cleanup + formatting tweak. |
| tests/bare_metal_server.rs | Updates witness test to new SubscriptionHandle::for_each_subscriber API + improves mock recv/timer behavior. |
| tests/bare_metal_e2e.rs | Adds new bare-metal client+server wiring integration tests with mock network. |
| tests/bare_metal_client_local.rs | Adds witness test for Client::new_with_deps_local + local spawner wiring. |
| tests/bare_metal_client.rs | Improves mock transport waking + timer contract adherence in bare-metal client witness. |
| src/transport.rs | Docs updates + adds LocalSpawner trait + small formatting tweaks. |
| src/tokio_transport.rs | Adjusts multicast loop option application semantics + doc cleanup. |
| src/static_channels/mod.rs | Improves close semantics for static channels (sender-side behavior) + adds tests. |
| src/server/subscription_manager.rs | Redesigns SubscriptionHandle (visitor API, relaxes Send bounds). |
| src/server/sd_state.rs | Switches SD offer send path to stack buffer to avoid per-tick allocation. |
| src/server/mod.rs | Back-fills ephemeral port into config, uses stack buffers for SD messages, improves error typing. |
| src/server/event_publisher.rs | Migrates to visitor-based subscriber iteration and stack snapshot buffer. |
| src/lib.rs | Updates feature table/docs and gates alloc behind embassy_channels; re-exports LocalSpawner. |
| src/embassy_channels.rs | Moves EmbassySyncChannels behind embassy_channels feature and implements close semantics. |
| src/client/socket_manager.rs | Adds local (!Send) bind variants and strengthens E2E protect failure behavior. |
| src/client/mod.rs | Introduces Client::new_with_deps_local and dispatch abstraction plumbing. |
| src/client/inner.rs | Refactors Inner to use bind dispatch trait instead of direct factory+spawner fields. |
| src/client/bind_dispatch.rs | New module abstracting bind+spawn for Spawner vs LocalSpawner. |
| examples/bare_metal_server/src/main.rs | Updates mock recv waking + timer behavior and subscription API usage. |
| examples/bare_metal_client/src/main.rs | Updates mock recv waking + timer behavior. |
| README.md | Updates feature descriptions and usage examples for new feature split. |
| Cargo.toml | Adds embassy_channels feature and new tests; updates feature docs. |
| CHANGELOG.md | Documents breaking API changes and new LocalSpawner/feature split. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fix documentation and unit test naming
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- cargo fmt: remove extra blank lines left by deleted deliver_inbound blocks - static_channels_alloc_witness: fix typo "heap-back" -> "heap-backed" - no_alloc_witness: doc says "panic"; impl actually calls process::abort() - CHANGELOG: bare_metal feature desc incorrectly listed EmbassySyncChannels; EmbassySyncChannels is gated by embassy_channels (which implies bare_metal) - CHANGELOG: document Server::unicast_local_addr breaking return-type change (Result<_, std::io::Error> -> Result<_, server::Error>) - tokio_transport: bind impl missing explicit + Send; add for clarity - tokio_transport: comment said bare_metal gates embassy_channels module; correct to embassy_channels feature - event_publisher: MAX_FANOUT duplicated SUBSCRIBERS_PER_GROUP; remove MAX_FANOUT and use pub(crate) SUBSCRIBERS_PER_GROUP from subscription_manager Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…9 Low) Critical: - C1: gate StaticE2EHandle/StaticE2EStorage behind cfg(all(bare_metal, std)); AtomicInterfaceHandle remains no_std. cargo build --no-default-features --features bare_metal now compiles. CI gate added. - C2: bump version to 0.8.0 so cargo-semver-checks classifies the breaking changes correctly; adds matching CHANGELOG section header. - C3: fix static-pool first-claim race in OneshotPool/MpscPool ensure_seeded (concurrent first claimers no longer panic with "pool exhausted"). New regression test asserts 4 concurrent first claims all succeed. High: - H1: replace single-slot AtomicWaker with MultiWakerRegistration<8> on the bounded send-close path in both static_channels and embassy_channels; cloned senders blocked on a full channel are all woken on receiver drop. New regression test covers multi-sender wake. - H2: pack (session_id, has_wrapped) into one AtomicU32 in SdStateManager; concurrent emitters around the 0xFFFF -> 0x0001 wrap boundary can no longer disagree. New stress test runs 32 concurrent emitters across 20 trials and asserts the (sid, flag) invariant. - H3: handle_sd_message now rolls back a committed subscription when the ACK send fails, and never propagates transient SD-socket I/O errors via ?, so a single SD hiccup cannot tear down run(). - H4: announcement_loop is now idempotent — second call returns Error::Io(InvalidInput) via an AtomicBool latch. - H5: validate event_group_id against ServerConfig::event_group_ids in the Subscribe handler; unknown groups now NACK with "unknown_event_group" instead of being silently ACKed (opt-in via populated Vec). - H6: convert Timer::sleep and TransportFactory::bind to GAT-based associated future types. Multi-threaded callers add for<'a> F::BindFuture<'a>: Send + for<'a> Tm::SleepFuture<'a>: Send; bare-metal !Send backends are no longer blocked. TokioTransport gets named TokioBindFuture and TokioSleep; tests use BoxFuture / Ready. - H7: SocketOptions::multicast_loop_v4 is now Option<bool>. Pinning an outbound interface no longer silently disables IP_MULTICAST_LOOP when the caller had no opinion. - H8: receive_any_unicast and receive_discovery now evict dead socket managers (poll_receive returns Ready(None)) instead of busy-looping on Error::SocketClosedUnexpectedly. - H9: re-enqueued Subscribe carries the just-bound unicast_port, so pass-2 hits the bind_unicast dedupe instead of leaking another ephemeral socket. - H10: split recv-error counter into transient/fatal classes via IoErrorKind::is_transient_recv. Inbound ICMP storms (ConnectionRefused), WouldBlock, Interrupted, TimedOut, NetworkUnreachable no longer count toward MAX_CONSECUTIVE_RECV_ERRORS. Added IoErrorKind::WouldBlock variant. - H11: rewrite intra-doc links that target feature-gated items as code literals. cargo doc with partial feature subsets is now warning-free; CI runs --features client and --features server,bare_metal doc builds with -D warnings. - H12: publish_event / publish_raw_event now return Err(Transport(_)) when every send failed, instead of masking total failure as Ok(0). Medium: - M1: rephrase CHANGELOG known-issue bullet to point at .config/nextest.toml (which serializes the client_server suite) instead of stale --test-threads=1 advice. - M3: clear stale waker registrations on slot release in OneshotPool / MpscPool so the next tenant's first registration cannot poke a defunct task. - M4: Client::set_interface(current_iface) is now a no-op; previously it silently bound the discovery socket as a side effect. - M5: SocketManager::shut_down drains the receiver until None instead of returning after one buffered message, ensuring the loop has actually dropped the underlying socket before we proceed. - M6: drop dead "overflow" branch in publish_event / publish_raw_event and add a const_assert tying the snapshot buffer cap to SUBSCRIBERS_PER_GROUP. - M8: document that register_e2e / unregister_e2e bypass the run-loop control channel and are therefore not subject to Error::Shutdown. - M9: Inner SendToService advances session_counter only on Ok send, so transient transport failure cannot chew through 16-bit space. - M10: lib.rs feature table now spells out that bare_metal alone is no_std-friendly, StaticE2EHandle additionally requires std, and embassy_channels users on no_std must wire up #[global_allocator]. - M11/M13: rewrite client::Error::Capacity tag list with one-line semantics for each tag and a note that "udp_buffer" can fire post-E2E-protect. Low: - AtomicInterfaceHandle uses Release/Acquire instead of Relaxed. - TokioSpawner::spawn wraps its future in catch_unwind and tracing::error!-logs panics so they are visible in the operator's log pipeline. - IoErrorKind::WouldBlock added; map_io_error routes std::io::ErrorKind::WouldBlock to it. - StaticUnboundedSender::send_now docstring documents the unified Err(value) for "closed" vs "full". - no_alloc_witness ARMED uses Acquire load (matches SeqCst stores) for weak-memory correctness. - transport.rs:1056 stale ControlMessage link rewritten as code literal. Deferred (with rationale documented in code/CHANGELOG): - M2 Client run-loop alloc witness — needs a custom no-alloc spawner harness; the existing static_channels_alloc_witness covers the channel layer. - L: configurable client_id, session_id move out of SocketManager, drop unused ChannelFactory bounds, route MTU through max_datagram_size — substantive API changes flagged for follow-up. Verification: - cargo fmt --check clean - cargo clippy --all-features --all-targets -- -D warnings -D clippy::pedantic clean - cargo doc --no-deps --all-features and partial-feature subsets clean - cargo nextest run --all-features: 524/524 pass, 8 skipped - cargo semver-checks check-release: no semver update required (0.7.0 -> 0.8.0) - 13-config build matrix: all green, including standalone bare_metal Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ture The previous commit landed the GAT-based future types on TransportFactory and Timer (H6 from the adversarial review), but missed the workspace example crates: cargo clippy --workspace --all-features (CI's command) exercises every workspace member, including the example binaries, so their mock TransportFactory / Timer impls also need the new associated types. Also adds the new ServerConfig::event_group_ids field (H5) to the client_server example via struct-update syntax over ServerConfig::new. Verified locally: - cargo build --workspace --all-features clean - cargo clippy --workspace --all-features -- -D warnings -D clippy::pedantic clean - cargo clippy --no-default-features -- -D warnings -D clippy::pedantic clean - cargo fmt --all --check clean Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 32 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Adds targeted unit tests for the five higher-leverage behaviors that landed in this PR's adversarial-review pass without coverage: - H10 (`io_error_kind_transient_classification`): pure-function check that `IoErrorKind::is_transient_recv` returns true for `ConnectionRefused` / `NetworkUnreachable` / `WouldBlock` / `Interrupted` / `TimedOut`, and false for `PermissionDenied` / `Other`. Locks in the classification driving the recv-loop fatal-error counter. - H5 (`server_config_accepts_event_group_*`): two pure-function tests on `ServerConfig::accepts_event_group` — empty `event_group_ids` accepts any group (back-compat), populated `event_group_ids` validates strictly. - H4 (`announcement_loop_second_call_returns_invalid_input`): builds a Server, calls `announcement_loop()` twice, asserts the second call returns `Err(Error::Io(InvalidInput))` with "already started" in the diagnostic. Prevents regressions of the AtomicBool latch. - H12 (`publish_event_returns_err_when_every_send_fails`, `publish_raw_event_returns_err_when_every_send_fails`): mock `TransportSocket` whose `send_to` always returns `Err(NetworkUnreachable)` / `Err(ConnectionRefused)`, registers a subscriber, calls `publish_event` / `publish_raw_event`, asserts the result is `Err(Transport(Io(_)))` rather than the previous `Ok(0)` masking total failure. - H3 (`handle_sd_message_rolls_back_subscription_on_failed_ack_send`): builds a Server via `new_with_deps` with a `FailingFactory` whose sockets always fail `send_to`. Drives a Subscribe through `handle_sd_message` and asserts the function returns `Ok(())` (the H3 fix log-and-continues instead of propagating via `?`) AND the subscription manager has been rolled back to 0 entries. Other already-covered behaviors (C3, H1, H2) had regression tests added in the previous commit; remaining client-side gaps (H8, H9, M4, M9) are deferred — each needs a sizable Client + mock harness and is better addressed as a separate test-infrastructure task. Verification: - cargo test --lib --all-features: 510 pass (was 503; +7 new tests) - cargo nextest run --all-features: 531/531 pass, 8 skipped - cargo clippy --workspace --all-features -- -D warnings -D clippy::pedantic clean - cargo clippy --no-default-features -- -D warnings -D clippy::pedantic clean - cargo fmt --all --check clean Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Both files were swept in by a `git add -A` in the previous commit and should not have been part of this PR's review-fix scope: - `.vscode/settings.json` — per-developer editor auto-approve list - `.github/copilot-instructions.md` — Copilot guidance file (also stale with respect to this PR's behavior changes; not the right time to refresh it) Removed via `git rm --cached`, so local working copies are preserved. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two unaddressed comments on PR #95 after the previous round of fixes: - `src/tokio_transport.rs:411-414` (TokioChannels docstring): said active under `client` or `server` features, but the entire `tokio_transport` module is `#[cfg(any(feature = "client-tokio", feature = "server-tokio"))]`. Reword to name the actual gating features and clarify the bare `client`/`server` features only expose the trait surface. - `tests/no_alloc_witness.rs:81` (cosmetic eprintln! trailing comma): the trailing comma after the format string is valid Rust but Copilot flagged it as syntactic noise. Removed for readability. Other comments in the latest batch were either already addressed or made obsolete by my prior commits: - The four "TransportFactory/Timer trait surface mismatch" comments on examples/bare_metal_{client,server}/src/main.rs (Copilot 21:32-21:33Z) pre-dated commit fe618cf, which ported those exact mocks to the new GAT pattern. Verified current state — the examples now match the trait signature. - All comments from earlier in the day already carry "Fixed" replies from a previous round. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot review on #95 flagged that `assert_no_alloc`'s doc still said forbidden allocations "panic" and exit with a non-zero status. The implementation actually routes through `diagnose_and_abort`, which disarms the allocator, writes the diagnostic to stderr, and then calls `std::process::abort()` — no unwinding. (Panicking would re-allocate via the panic-unwind machinery and re-trip the trap, which is exactly why we abort instead.) Updated the docstring to match the abort semantics so CI failures are interpreted correctly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 32 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+181
to
+182
| // 16-byte SOME/IP header + the SD payload, capped at the UDP | ||
| // datagram limit. |
There was a problem hiding this comment.
The comment says the buffer is “capped at the UDP datagram limit”, but crate::UDP_BUFFER_SIZE is an application-level cap (1500) and not the actual UDP payload limit (~65KiB). Please reword to avoid implying the buffer matches the protocol limit (and to prevent future readers from assuming larger SD payloads are supported here).
Suggested change
| // 16-byte SOME/IP header + the SD payload, capped at the UDP | |
| // datagram limit. | |
| // 16-byte SOME/IP header + the SD payload, capped by this | |
| // application's fixed `crate::UDP_BUFFER_SIZE` buffer size. |
Comment on lines
202
to
209
| /// | ||
| /// # Socket bounds | ||
| /// | ||
| /// Phase 12 relaxed the previous `F::Socket = TokioSocket` pin by | ||
| /// switching [`TransportSocket`] to GATs. The factory's socket type | ||
| /// must now satisfy: | ||
| /// [`TransportSocket`] uses GATs so the factory's socket type must | ||
| /// satisfy: | ||
| /// | ||
| /// - `Send + Sync + 'static` — so the socket loop future can be | ||
| /// spawned on a multithreaded executor and outlive its owner. |
There was a problem hiding this comment.
The “Socket bounds” docs describe the spawned socket and its send/recv futures being Send, but with the new TransportFactory::BindFuture<'a> GAT, this function also needs for<'a> F::BindFuture<'a>: Send to remain spawnable on multi-thread runtimes. Without that bound, a backend can compile here but still make bind_discovery_seeded_with_transport’s returned future !Send (because it awaits factory.bind(..)), which then fails when called from a Send run-loop. Consider adding the bound and updating this doc list to include it.
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.
fix: phase 17 cleanup - docs, alloc gating, bare_metal_e2e test
embassy_channelsfeature to gate alloc (separate from bare_metal)