Max buffer size set to 1500, avoid heap allocation/vecs, overflow ret…#77
Open
JustinKovacich wants to merge 9 commits into
Open
Conversation
JustinKovacich
added
documentation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR standardizes outbound UDP buffer sizing to a crate-wide constant and refactors client/server send paths to use fixed-size stack buffers (avoiding heap allocation), while adding explicit error handling and tests for oversize datagrams.
Changes:
- Introduces
UDP_BUFFER_SIZE = 1500and updates client/server send paths to encode into fixed[u8; UDP_BUFFER_SIZE]buffers. - Adds capacity/overflow handling for E2E-protected and raw-event publishes (returning
Error::Capacity("udp_buffer")). - Adds unit tests covering oversize raw-event publish and E2E-protected oversize send behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/server/event_publisher.rs | Switches event publishing to fixed-size buffers; adds oversize raw-event test and E2E overflow handling. |
| src/server/error.rs | Adds Capacity variant and marks server error enum #[non_exhaustive]. |
| src/lib.rs | Adds the public UDP_BUFFER_SIZE constant and documentation. |
| src/client/socket_manager.rs | Switches socket loop buffer to fixed-size array; adds E2E oversize test and capacity error return. |
| src/client/mod.rs | Updates client memory-footprint documentation to account for the new buffers. |
| src/client/error.rs | Updates Capacity documentation to include the new "udp_buffer" tag. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This was referenced Apr 22, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 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 6 out of 6 changed files in this pull request and generated 2 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 6 out of 6 changed files in this pull request and generated 3 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 24, 2026
Three doc-only Copilot round-3 nits on PR #77: - src/client/mod.rs: the per-`SocketManager` memory-footprint blurb implied the second `[u8; UDP_BUFFER_SIZE]` is always allocated. In fact `socket_loop_future` only allocates that scratch buffer when the send path actually needs E2E protection (the destination key is in the `E2ERegistry`); plain sends pay only ~1.5 KiB. Reworded the always-live vs peak budget so the ~24 KiB number is no longer presented as the steady-state cost. - src/client/socket_manager.rs: the E2E-overflow test comment said "8 over MTU", but `UDP_BUFFER_SIZE` is documented as a UDP payload cap, not an Ethernet-MTU-safe size. Reworded to "8 bytes over UDP_BUFFER_SIZE". - src/lib.rs: the `UDP_BUFFER_SIZE` doc referenced bare `Error::Capacity("udp_buffer")`, which is ambiguous at the crate root (no `crate::Error` exists). Qualified to `client::Error::Capacity(...)` / `server::Error::Capacity(...)`. Addresses Copilot comments 3138697909, 3138698042, 3138698156. 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 6 out of 6 changed files in this pull request and generated 1 comment.
💡 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 6 out of 6 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 6 out of 6 changed files in this pull request and generated 1 comment.
💡 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 6 out of 6 changed files in this pull request and generated 7 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 6 out of 6 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
force-pushed
the
feature/move_vec_behind_feature_gate
branch
from
505bf8f to
bceae77
Compare
…urns an error, added unit tests
Commit 343da67 added a `required_size() > UDP_BUFFER_SIZE` pre-check to `EventPublisher::publish_event` but left the new branch uncovered. Regression guard added as `publish_event_pre_encode_exceeds_udp_buffer_returns_capacity_error`: registers a subscriber (the pre-check sits after the `subscribers.is_empty()` early return, so the test needs one or else hits the false-positive Ok(0) path), constructs a 1501-byte fixture (16-byte header + 1485-byte payload, one over the cap), calls publish_event, asserts Err(Error::Capacity("udp_buffer")). Mirrors the fixture pattern from `send_raw_message_exceeding_udp_buffer_returns_capacity_error` on the client side. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- src/lib.rs: UDP_BUFFER_SIZE doc now enumerates exactly which send paths honor this cap (SocketManager::send, publish_event, publish_raw_event) and explicitly calls out the SD announcement / SubscribeAck / SubscribeNack paths that still use heap Vec buffers as a known gap planned for the bare-metal no_alloc refactor. - src/server/event_publisher.rs: reworded "stack buffer sized to MTU" comments at the two buffer-allocation sites — the buffer lives in the async future's state, not literally on the stack, and the cap is a UDP payload limit, not an Ethernet MTU. New wording points at the UDP_BUFFER_SIZE docs for the distinction. The two `use std::vec` comments (event_publisher.rs:335, socket_manager.rs:362) were verified to be false positives: removing the imports breaks the lib-test build with 4 errors about `vec!` macro not in scope. Same no_std mechanics as the prior #75-1 resolution — reply posted on the comment threads. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Three doc-only Copilot round-3 nits on PR #77: - src/client/mod.rs: the per-`SocketManager` memory-footprint blurb implied the second `[u8; UDP_BUFFER_SIZE]` is always allocated. In fact `socket_loop_future` only allocates that scratch buffer when the send path actually needs E2E protection (the destination key is in the `E2ERegistry`); plain sends pay only ~1.5 KiB. Reworded the always-live vs peak budget so the ~24 KiB number is no longer presented as the steady-state cost. - src/client/socket_manager.rs: the E2E-overflow test comment said "8 over MTU", but `UDP_BUFFER_SIZE` is documented as a UDP payload cap, not an Ethernet-MTU-safe size. Reworded to "8 bytes over UDP_BUFFER_SIZE". - src/lib.rs: the `UDP_BUFFER_SIZE` doc referenced bare `Error::Capacity("udp_buffer")`, which is ambiguous at the crate root (no `crate::Error` exists). Qualified to `client::Error::Capacity(...)` / `server::Error::Capacity(...)`. Addresses Copilot comments 3138697909, 3138698042, 3138698156. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…event
Adds a unit test that registers a Profile4 E2E profile for the message
key and publishes a message whose raw encoded size fits UDP_BUFFER_SIZE
(1496 bytes) but whose protected size does not (1508 bytes, after the
12-byte Profile4 header). Asserts that publish_event returns
Error::Capacity("udp_buffer") — exercising the post-protect guard that
was previously only covered on the client send path.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- publish_event: log now says "E2E-protected datagram (... header + protected payload)" so the 16+protected_len value is identified as the full SOME/IP datagram size, not the payload. - test fixture comment: "8 over MTU" → "8 bytes over UDP_BUFFER_SIZE" for terminology consistency with the rest of the PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…_len
`header_len + payload.len()` used unchecked `usize` addition. On a
system with large enough `payload.len()` the sum can wrap, silently
bypassing the `> UDP_BUFFER_SIZE` guard and corrupting the slice
operations that follow. Switch to `checked_add` and treat overflow the
same as exceeding `UDP_BUFFER_SIZE` — return
`Error::Capacity("udp_buffer")`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Client/server "E2E-protected payload" logs now say "E2E-protected datagram (header + protected payload)" since the logged value is the full SOME/IP datagram size, not the payload. - publish_raw_event checked_add-fail log now describes the real condition (usize overflow) and includes the input lengths, instead of falsely pointing at UDP_BUFFER_SIZE. - Oversize fixtures in socket_manager + event_publisher tests are now sized from UDP_BUFFER_SIZE (and PROFILE4_HEADER_SIZE implicitly for the E2E case) instead of hardcoded 1480/1485. Fixtures stay valid if the cap is retuned. - client/mod.rs memory-footprint doc no longer hardcodes "(1500 bytes)" or "~1.5 KiB" as load-bearing numbers; the scaling is now expressed in terms of UDP_BUFFER_SIZE with the current-default numbers as a parenthetical reference. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
JustinKovacich
force-pushed
the
feature/stack_buffer_encoding_bufs
branch
from
e68f013 to
f5bf200
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 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.
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.
…urns an error, added unit tests
This is a chained PR. Prev: #76. Next: #78