feat(server)!: multi-response handler, listener injection, UDP identification (0.4.0) - #6
Merged
Merged
Conversation
…rning one ServerConnectionHandler::diagnostic_message returned exactly one OwnedMessage, so a handler could never emit the DiagnosticMessageAck that ISO 13400 requires before a UDS response — uds_on_ip blocks waiting for that ack, which made this server unusable for driving a real UdsClient. Pass a &mut dyn ResponseWriter instead. Each send goes straight to the socket, so a handler can also await between writes and hold an NRC 0x78 pending wait open rather than emitting a burst. BREAKING CHANGE: diagnostic_message takes a responses writer and returns Result<(), Error>. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
README's known-limitations list and ARCHITECTURE section 7.1 both described ServerConnectionHandler::diagnostic_message as unable to emit an ack followed by a response. The preceding commit gave it a ResponseWriter, so both entries now describe behavior the crate no longer has. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Guards the design decision behind ResponseWriter: a Vec<OwnedMessage> return would drain back-to-back and make the elapsed-time assertion here impossible, so this test fails if the sink is ever replaced by a batch return. Also folds the six near-identical positive-ack bodies in this file into one `send_positive_ack` helper, so a new handler no longer re-derives the ack. The helper addresses the ack from the entity to the requesting tester, which is the direction ISO 13400 specifies and the one the multi-response handlers already used; the four handlers that had the addresses reversed now match. Nothing asserts on the ack's addresses and the client ignores them, so behavior is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The >=100ms bound could not fail for the regression it guarded. The two 50ms sleeps are handler logic, so a batched Vec<OwnedMessage> rewrite keeps them: the handler still takes ~100ms, the four messages are just written together at the end, and the final one still lands at ~100ms. Measured under a simulated batch: total 102.77ms, i.e. green. What separates the designs is interleaving, so assert on that. The ack must arrive before the handler's first sleep (<40ms; 50us streamed, 102.75ms batched), and the two pending responses must be >=40ms apart (51.3ms streamed, 4.3us batched). The total-time check stays as a guard against a fixture that stops sleeping, re-commented to say it does not discriminate the two designs. Both bounds are stated against INTERLEAVING_MARGIN, documented as raise-never- delete if it proves tight under CI load. Disabling Nagle on the accepted socket is required to observe any of this: ConnectorSocket sets TCP_NODELAY for the client but nothing sets it for an accepted connection, so consecutive small responses stalled ~40ms on the tester's delayed ACK - the pending gap measured 7.3ms through a genuinely streamed sink. The fixture sets it; whether run_server should is left open. Also assert the routing activation response arrives before the diagnostic exchange, so a broken activation reports itself instead of surfacing as a missing ack, and abort the accept loop like the neighboring test does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…errors
run_server hardcoded bind("0.0.0.0", 13400), so an entity could not be placed
on a loopback alias and tests could not use an ephemeral port in parallel.
run_server_with_listener takes a listener the caller already bound; run_server
delegates to it and is unchanged for existing callers.
The accept loop also panicked on any accept error. Transient conditions must
not take the entity down, so log and continue.
Accepted sockets never had TCP_NODELAY set, though ConnectorSocket sets it
client-side. Consecutive small frames - an ack then a response, or successive
NRC 0x78 pendings - waited on the peer's delayed ACK, costing up to ~40ms of
P2 budget per exchange (measured 43.6ms in the response-pending test). Set it
on every accepted connection and drop the test fixture's workaround, so the
interleaving guard measures shipped behavior.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The section promised an Err on fatal handler failure, but the loop logs every error and never breaks, so the future never resolves and a caller would build recovery logic on a dead branch. Say so plainly, and record why the Result is kept: run_server needs a matching return type to propagate its bind failure, and a future shutdown path needs somewhere to report one. Also note on handle_client_connection that it sets TCP_NODELAY, since it is public and overrides a caller-configured stream, and align the two new accept loop tests with the file's convention of aborting the spawned server task. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
run_server carried a "TODO: Vehicle Announcement over UDP" and the TCP VehicleIdentificationRequest arm only warned, so a DoIP entity built on this crate was invisible to any UDP discovery probe. run_udp_responder serves a caller-bound UdpSocket, delegating the response content to the existing received_vehicle_identification_request hook. Malformed datagrams are logged and skipped. The reply is stamped with PayloadType::VehicleAnnouncement (0x0004): ISO 13400-2 defines a single wire payload type for both the unsolicited announcement and the directed reply, and there is no PayloadType::VehicleIdentificationResponse. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…equests Payload::decode collapses the plain, with-EID, and with-VIN request forms into one variant and drops the EID/VIN bytes, so the responder answers a directed request that named a different entity and never consults the vehicle_identification_with_eid / _with_vin hooks. Reading run_udp_responder next to those hooks otherwise suggests they are wired up. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…directed probes Only decode failures were skipped; socket and handler errors returned. That let any host on the network end discovery for the life of the process: on Windows an oversized datagram fails recvfrom with WSAEMSGSIZE rather than truncating, so a 2 KB packet killed the loop. A transient send failure (ENETUNREACH, firewall EPERM) or a handler that could not yet read its identity out of NVM did the same. Every failure in the loop is now logged and skipped, matching the TCP accept loop, so run_udp_responder no longer returns. The responder also answered VehicleIdentificationRequestWithEID/WithVIN probes unconditionally, because Payload::decode collapses all three request forms and drops the EID/VIN bytes. Answering a probe addressed to another entity actively misleads a tester, while staying silent degrades to a discovery timeout testers already handle, so the header payload type is now checked and the directed forms are declined. The known-limitation note states that trade-off instead of implying a Payload change was the only option. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Breaking: ServerConnectionHandler::diagnostic_message takes a ResponseWriter. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
README's Status list and ARCHITECTURE §7.6 both still said the server answers no vehicle-identification requests over UDP and that a failed `accept()` panics the server task. `run_udp_responder` answers the broadcast form, and the accept loop has logged-and-continued since e9cc750, so both claims were wrong in the two files a new integrator reads first. - Split the UDP claim into what is still true (no unsolicited announcement at power-on) and what replaced the rest, including that the caller must drive `run_udp_responder` itself because `run_server` binds TCP alone, and that only the broadcast `0x0001` form is answered. - Drop the accept-panic clause; note the current behavior in §7.6. - Point the one-connection-at-a-time entry at `run_server_with_listener`, where the loop now lives, and name the consequence a sim author cares about: a stalled tester wedges the entity. - Retire the dangling "single-response handler limitation" cross- reference in the examples section — that limitation was removed in 964ab24 and the anchor it pointed at no longer exists. Describe what `echo_server` actually does instead. - Reword §7's preamble so "none of it is scheduled" no longer contradicts §7.1, which is marked RESOLVED. - List `tests/udp_identification.rs` and `ResponseWriter` in the module and test maps, which the branch left out. - Date the 0.4.0 changelog entry as Keep a Changelog wants, and say the file begins at 0.4.0 so a reader does not read the missing 0.1-0.3 history as "nothing changed". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both non-fatal error paths — the TCP accept loop and the UDP responder's `recv_from` — retried immediately. The accept loop's own comment named its counterexample: it cited EMFILE as transient, but descriptor exhaustion persists until something else in the process releases an fd, and until then `accept` returns `Err` immediately on every iteration, forever. The panic this replaced was bad but loud and terminal; an unbounded error-log flood pegging a core is harder to diagnose in an unattended simulator. Sleep 100 ms in each `Err` arm before continuing — the standard accept-loop mitigation. A genuinely transient error costs one interval; a persistent one is bounded to ten retries a second. Also reword the accept-loop comment so it no longer implies every error it handles is transient. Untested by design: simulating descriptor exhaustion would destabilize the suite, and this is a strict improvement to an already-untested branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three gaps a caller only finds by reading the implementation: - `run_server_with_listener` sells the multi-entity-on-loopback-aliases case in detail but never says the loop serves one connection to completion before accepting the next. A sim author reading only this method builds exactly the topology — several entities, several testers — where one hung tester silently wedges an entity. State it, and point at README's Status section rather than repeating the explanation. - `run_server`'s rendered doc said nothing about UDP; the only disclosure was a `//` comment in the body, which rustdoc never emits. An entity started through the default entry point is invisible to a discovery probe and nothing said so. Add a `# Discovery` section, and since both methods take `&self`, show the `try_join!` composition rather than leaving the reader to derive it. - `run_udp_responder` said the socket is bound by the caller without saying that receiving broadcast probes requires binding `0.0.0.0`, not a specific address — the non-obvious half of that sentence. Also widen `Server`'s struct doc, which still described the type as TCP-only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An entity bound to `0.0.0.0:13400` sees every DoIP datagram on the network. A payload type this responder does not answer, and a directed identification probe it deliberately declines, are both ordinary traffic on a live bus — a tester doing directed discovery produces the latter as a matter of course. Warning about them makes a healthy entity look broken and buries the two cases that are genuinely worth a warning. Decode failures and send failures keep `warn!`: those describe a peer sending malformed bytes or an answer that did not get out, neither of which is expected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`handler_can_emit_ack_then_response` and the held-pending test both matched only `OwnedPayload::DiagnosticMessageAck(_)`. `ack_code` is a public field and both handlers send `RoutingConfirmationAck` through `send_positive_ack`, so checking it is one line each. The variant-only check was load-bearing on a bug: the ack constructors hardcode the positive payload type regardless of the code (ARCHITECTURE §7.2), so a handler that regressed to a negative code would still produce a frame these assertions accept. Asserting the code decouples the tests from that hardcode surviving unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The `# Discovery` snippet on `run_server` was an `ignore` block, which rustdoc never compiles — exactly the kind of sample that rots into something that no longer builds. Hidden lines wrapping it in a function generic over the handler make it a real doctest without needing a concrete `ServerConnectionHandler` in the example. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…elog The # Discovery section said "neither returns", contradicting run_server's own # Errors block three lines below — a bind failure does propagate. Say what was meant: neither completes normally, and this one can still return early on a bind failure. The 0.4.0 Fixed entry also described the accept loop as "logs and continues" without the 100ms backoff that shipped with it, and omitted the UDP log-level demotions entirely. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The changelog was not asked for and this repo did not have one. The 0.4.0 migration note lives in the PR body and the release notes instead. The version bump from the same commit stays — only the file is removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Cargo.toml went to 0.4.0 but the lockfile still pinned simple_doip 0.3.0, so `cargo publish --dry-run` re-resolved and rewrote Cargo.lock before checking the tree — then failed the CI `package` job with "1 files in the working directory contain changes that were not yet committed into git". Only this crate's own version entry changes; no dependency pins move. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
JustinKovacich
force-pushed
the
feature/server-multi-response
branch
from
August 13, 2026 14:03
ead6d81 to
ff5085a
Compare
zheylmun
approved these changes
Aug 19, 2026
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
2 participants
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.
What
Makes
simple_doip::serverable to drive a real UDS tester, and releases0.4.0.The blocker was structural:
ServerConnectionHandler::diagnostic_messagereturned exactly oneOwnedMessage, so a handler could never emit theDiagnosticMessageAckthatuds_on_ipblocks on before reading a response. No handler could satisfy a real tester, which is why downstream consumers hand-roll raw-frame fake ECUs instead of using this crate's server.diagnostic_messagetakes&mut dyn ResponseWriterand returnsResult<(), Error>ResponseWriter,Server::run_server_with_listener,Server::run_udp_responder,OwnedMessage::vehicle_identification_responseTCP_NODELAYon accepted sockets, fatal errors in the UDP loopWhy each piece
Response sink, not a batched return. A
Vec<OwnedMessage>return would drain back-to-back and could not express a held NRC0x78pending wait — the handler must be able to emit a pending, await real work, then emit the response.ResponseWriter::sendreaches the socket immediately, so it can.TCP_NODELAYon accepted sockets.ConnectorSocketset it client-side; an accepted socket did not. Consecutive small frames — an ack then a response, or successive pendings — waited on the peer's delayed ACK. Found because the interleaving test failed against a correct implementation: pending #1 arrived at 42.8 ms with the two pendings only 8.3 ms apart. After the fix, ack arrival is 49–56 µs and the gap tracks the handler's real 50 ms sleep.Backoff on the non-fatal loops. Not every accept error is transient — descriptor exhaustion persists, so retrying without a pause trades a loud panic for a pegged core and an unbounded log flood.
Declining directed probes.
Payload::decodecollapses payload types0x0001/0x0002/0x0003and discards the EID/VIN bytes, so the responder cannot tell whether it is the addressee. Answering anyway means every entity on a network replies to a tester's directed probe. Silence degrades to a timeout, which testers already handle. Broadcast0x0001is unaffected. A correct implementation needsPayloadto preserve those bytes;vehicle_identification_with_eid/_with_vinremain unconsulted until it does (they already were).Migration
Handlers returning
Ok(msg)becomeresponses.send(msg).await?; Ok(()).examples/echo_server.rsis the worked ack-then-response conversion. There are no in-tree implementors outside this crate.Testing
64 tests, all green;
clippy --tests,fmt --check, andRUSTDOCFLAGS=-Dwarnings cargo docclean;--no-default-featuresstill builds theno_stdcore.Three tests were verified by deliberately making them fail rather than trusting a green run:
ack_at=102.75 ms,gap=4.3 µs(red). An earlier total-elapsed assertion still passed under that same mutation, which is why it was replaced: the sleeps are handler logic either way, so total duration cannot discriminate a sink from a batch.The accept-error branch ships untested: triggering a real
accept()failure means exhausting file descriptors, which would destabilize the suite, and a trait seam overTcpListener::acceptis real abstraction weight for a three-line log-and-continue. The limitation is stated in the covering test's own doc comment.Reviewer notes
Consumers must bump their version requirement in the same commit as the submodule pointer.
0.4.0fails a^0.3requirement, which red-lines the whole consuming workspace, not just the DoIP crates.run_serverbinds TCP only. An entity that wants to be discoverable drives both it andrun_udp_responder;run_server's# Discoverysection carries thetokio::try_join!composition.Known, unchanged by this PR: the accept loop still awaits each connection inline, so one connection blocks the next and a hung tester wedges the entity. Pre-existing and documented in the README, but
run_server_with_listenernewly makes that shape public — worth a decision before this is used as a multi-entity sim host.Opened as a draft for human review.