Skip to content

Fix decoder example to propagate errors #192

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
leynos merged 6 commits into from
Jul 7, 2025
Merged

Fix decoder example to propagate errors #192

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ee23219
Refine length-prefixed decoding example
leynos Jul 6, 2025
dcdce6b
Handle invalid frame sizes
leynos Jul 6, 2025
929e723
Improve frame processor example
leynos Jul 6, 2025
347e9a9
Explain error propagation
leynos Jul 6, 2025
597302a
Fix decoder buffer checks
leynos Jul 7, 2025
2134dc0
Merge branch 'main' into codex/update-decoder-example-to-handle-errors
leynos Jul 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 44 additions & 47 deletions docs/rust-binary-router-library-design.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1120,65 +1120,62 @@ examples are invaluable. They make the abstract design tangible and showcase how
}
```

2. **Frame Processor Implementation** (Simple Length-Prefixed Framing using
`tokio-util`):
1. **Frame Processor Implementation** (Simple length-prefixed framing using
`tokio-util`; invalid input or oversized frames return `io::Error` from both
decode and encode):

Comment on lines +1123 to 1126
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Jul 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Wrap prose at 80 columns to satisfy documentation linting

The bullet text introducing the codec example runs far beyond the 80-column limit mandated for docs/**/*.md. This will fail markdownlint/CI.

🤖 Prompt for AI Agents 
In docs/rust-binary-router-library-design.md around lines 1123 to 1126, the
bullet point text exceeds the 80-column limit required by markdownlint. Reformat
the text by wrapping the prose so that no line exceeds 80 characters, ensuring
it meets the documentation linting standards without altering the content.

```rust
// Crate: my_frame_processor.rs
use bytes::{BytesMut, Buf, BufMut};
use tokio_util::codec::{Decoder, Encoder};
use std::io;
```rust
// Crate: my_frame_processor.rs
use bytes::{BytesMut, Buf, BufMut};
use tokio_util::codec::{Decoder, Encoder};
use byteorder::{BigEndian, ByteOrder};
use std::io;

const MAX_FRAME_LEN: usize = 16 * 1024 * 1024; // 16 MiB upper limit
const MAX_FRAME_LEN: usize = 16 * 1024 * 1024; // 16 MiB upper limit

pub struct LengthPrefixedCodec;
pub struct LengthPrefixedCodec;

impl Decoder for LengthPrefixedCodec {
type Item = BytesMut; // Raw frame payload
type Error = io::Error;
impl Decoder for LengthPrefixedCodec {
type Item = BytesMut; // Raw frame payload
type Error = io::Error;

fn decode(&mut self, src: &mut BytesMut) -> Result<Option<Self::Item>, Self::Error> {
if src.len() < 4 { return Ok(None); } // Not enough data for length prefix
fn decode(&mut self, src: &mut BytesMut) -> Result<Option<Self::Item>, Self::Error> {
if src.len() < 4 { return Ok(None); } // Not enough data for length prefix

let length = u32::from_be_bytes(
src[..4].try_into().expect("slice length checked"),
) as usize;
let length = BigEndian::read_u32(&src[..4]) as usize;

if length > MAX_FRAME_LEN {
return Err(io::Error::new(io::ErrorKind::InvalidInput, "frame too large"));
}
if length > MAX_FRAME_LEN {
return Err(io::Error::new(io::ErrorKind::InvalidInput, "frame too large"));
}

if src.len() < 4 + length {
src.reserve(4 + length - src.len());
return Ok(None); // Not enough data for full frame
}
if src.len() < 4 + length {
src.reserve(4 + length - src.len());
return Ok(None); // Not enough data for full frame
}
src.advance(4); // Consume length prefix
Ok(Some(src.split_to(length)))
}
}

Copy link
Copy Markdown

@SourceryAI SourceryAI Jul 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (review_instructions): Advance the buffer by 4 bytes to consume the length prefix before splitting out the frame payload.

Currently, you split out the payload without first advancing past the 4-byte length prefix, so the returned BytesMut will include the length prefix. You should call src.advance(4) before split_to(length).

Review instructions:

Path patterns: **/*

Instructions:
Create code-review comments for ALL issues. Avoid making general observations or non-specific feedback if at all possible.

Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai Bot Jul 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (review_instructions): Advance the buffer by 4 bytes to consume the length prefix before splitting the payload.

You must call src.advance(4) before src.split_to(length) to remove the length prefix from the buffer. Otherwise, the returned frame will include the length prefix, which is incorrect.

Review instructions:

Path patterns: **/*

Instructions:
Create code-review comments for ALL issues. Avoid making general observations or non-specific feedback if at all possible.

src.advance(4); // Consume length prefix
Ok(Some(src.split_to(length)))
}
}
impl<T: AsRef<[u8]>> Encoder<T> for LengthPrefixedCodec {
type Error = io::Error;

impl<T: AsRef<[u8]>> Encoder<T> for LengthPrefixedCodec {
type Error = io::Error;

fn encode(&mut self, item: T, dst: &mut BytesMut) -> Result<(), Self::Error> {
let data = item.as_ref();
let length = u32::try_from(data.len()).map_err(|_| {
io::Error::new(
io::ErrorKind::InvalidInput,
"payload exceeds 4 GiB limit",
)
})?;
dst.reserve(4 + data.len());
dst.put_u32(length);
dst.put_slice(data);
Ok(())
fn encode(&mut self, item: T, dst: &mut BytesMut) -> Result<(), Self::Error> {
let data = item.as_ref();
if data.len() > MAX_FRAME_LEN {
return Err(io::Error::new(io::ErrorKind::InvalidInput, "frame too large"));
}
}
```

(Note: "wireframe" would abstract the direct use of `Encoder`/`Decoder` behind
its own `FrameProcessor` trait or provide helpers.)
dst.reserve(4 + data.len());
dst.put_u32(data.len() as u32);
dst.put_slice(data);
Ok(())
}
}
Comment on lines +1128 to +1174
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Jul 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Introduce a LEN_PREFIX constant for clarity & easier maintenance.

The literal 4 appears several times and couples the logic tightly to a
32-bit length prefix. Replacing the magic number with a named constant improves
readability and makes it trivial to swap in a different prefix width later.

@@
-        if src.len() < 4 { return Ok(None); } // Not enough data for length prefix
+        const LEN_PREFIX: usize = 4;
+        if src.len() < LEN_PREFIX {
+            return Ok(None); // Not enough data for length prefix
+        }
@@
-        if src.len() < 4 + length {
+        if src.len() < LEN_PREFIX + length {
             src.reserve(LEN_PREFIX + length - src.len());
             return Ok(None); // Not enough data for full frame
         }
-        src.advance(4); // Consume length prefix
+        src.advance(LEN_PREFIX); // Consume length prefix
@@
-        dst.reserve(4 + data.len());
-        dst.put_u32(data.len() as u32);
+        dst.reserve(LEN_PREFIX + data.len());
+        dst.put_u32(data.len() as u32);

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In docs/rust-binary-router-library-design.md between lines 1128 and 1174,
replace all occurrences of the literal 4, which represents the length prefix
size, with a new constant named LEN_PREFIX. Define LEN_PREFIX as 4 at the top of
the file to improve code clarity and maintainability. Update all length prefix
related calculations and buffer operations to use LEN_PREFIX instead of the
hardcoded 4.

```

(Note: "wireframe" would abstract the direct use of `Encoder`/`Decoder` behind
its own `FrameProcessor` trait or provide helpers.)
Comment on lines +1177 to +1178
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Jul 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Line length exceeds documentation guidelines

docs/**/*.md must wrap prose at 80 columns, but the note starting with
“Note: "wireframe" would abstract…” runs far longer. Please re-wrap or let
mdformat-all handle it so CI markdown-lint passes.

🤖 Prompt for AI Agents 
In docs/rust-binary-router-library-design.md around lines 1178 to 1179, the note
about "wireframe" exceeds the 80-column line length limit for documentation.
Reformat the text to wrap lines at or below 80 characters, either manually or by
running the mdformat-all tool, to ensure it complies with markdown linting
rules.


1. **Server Setup and Handler**:

Expand Down
Loading