buffer-param #154
buffer-param #154
Conversation
📝 WalkthroughWalkthroughThis pull request removes the Changes
Sequence Diagram(s)mermaid Handler->>Serializer: initialize with body and headers Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@include/boost/beast2/server/route_handler_corosio.hpp`:
- Around line 115-131: write_impl currently truncates if buffers.copy_to(bufs,
max_bufs) exceeds max_bufs and ignores partial writes from stream.write_some;
fix by processing the full buffer_sequence in chunks: repeatedly copy up to
max_bufs via buffers.copy_to(...) (using an offset/iterator or repeatedly
removing copied prefix) and for each capy::mutable_buffer attempt to write until
its bytes are consumed by looping on stream.write_some and advancing the buffer
by the returned written count on partial writes, retrying on transient errors
and returning ec on fatal errors; update references in write_impl to use
buffers.copy_to, capy::mutable_buffer bufs[], and stream.write_some when
implementing this chunked+retry logic.
- Around line 99-105: The Transfer-Encoding check currently uses exact equality
on res.value_or(http::field::transfer_encoding, "") which fails for
comma-separated or mixed-case tokens; update the condition to parse the header
string returned by res.value_or(...), split on commas, trim whitespace, and
perform a case-insensitive comparison of each token to "chunked" (so strings
like "gzip, chunked" or "Chunked" are detected). Replace the simple equality
test near the write_some call (the block using res and terminator) with this
tokenized/case-insensitive check and only send the "0\r\n\r\n" terminator when a
"chunked" token is present.
- Around line 64-87: The loop that writes serializer buffers uses
stream.write_some inside route_handler_corosio.hpp but ignores partial writes:
it sums buf.size() and calls serializer.consume(written) instead of the actual
bytes sent, which can drop data; update the write loop to handle partial writes
per buffer by retrying until each buffer's bytes are fully written (adjusting
the buffer pointer/offset passed to capy::const_buffer based on bytes written),
accumulate the actual bytes written into a variable (using the returned n from
stream.write_some) and call serializer.consume(actual_written) after sending all
bytes or on error; reference the serializer object, the for-loop over bufs,
stream.write_some, capy::const_buffer, and serializer.consume when making the
change.
| while(! serializer.is_done()) | ||
| { | ||
| auto rv = serializer.prepare(); | ||
| if(! rv) | ||
| co_return rv.error(); | ||
|
|
||
| auto bufs = *rv; | ||
|
|
||
| // Write all buffers | ||
| for(auto const& buf : bufs) | ||
| { | ||
| auto [ec, n] = co_await stream.write_some( | ||
| capy::const_buffer(buf.data(), buf.size())); | ||
| if(ec) | ||
| co_return ec; | ||
| } | ||
|
|
||
| // Calculate total size written | ||
| std::size_t written = 0; | ||
| for(auto const& buf : bufs) | ||
| written += buf.size(); | ||
|
|
||
| serializer.consume(written); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the specific file
find . -name "route_handler_corosio.hpp" -type fRepository: cppalliance/beast2
Length of output: 180
🏁 Script executed:
# Check the actual code at the specified lines
if [ -f "include/boost/beast2/server/route_handler_corosio.hpp" ]; then
wc -l "include/boost/beast2/server/route_handler_corosio.hpp"
fiRepository: cppalliance/beast2
Length of output: 119
🏁 Script executed:
# Get the context around lines 64-87
if [ -f "include/boost/beast2/server/route_handler_corosio.hpp" ]; then
sed -n '50,100p' "include/boost/beast2/server/route_handler_corosio.hpp" | cat -n
fiRepository: cppalliance/beast2
Length of output: 1896
🏁 Script executed:
# Look for write_some function signature and usage in the codebase
rg "write_some" --type cpp -B 2 -A 2 | head -80Repository: cppalliance/beast2
Length of output: 5321
🏁 Script executed:
# Check what capy::const_buffer is and stream types
rg "class.*stream|write_some|const_buffer" --type hpp --type cpp | grep -E "(stream|write_some)" | head -40Repository: cppalliance/beast2
Length of output: 89
🏁 Script executed:
# Verify write_some semantics by checking http_stream.hpp for pattern comparison
sed -n '1,50p' "include/boost/beast2/server/http_stream.hpp" | cat -nRepository: cppalliance/beast2
Length of output: 2025
🏁 Script executed:
# Get more context from http_stream.hpp to see if it has the same issue
rg "write_some|consume" "include/boost/beast2/server/http_stream.hpp" -B 3 -A 3Repository: cppalliance/beast2
Length of output: 410
🏁 Script executed:
# Let's get the exact code around lines 64-87 to confirm the bug
sed -n '60,90p' "include/boost/beast2/server/route_handler_corosio.hpp" | cat -nRepository: cppalliance/beast2
Length of output: 1110
🏁 Script executed:
# Compare with http_stream.hpp's pattern - get full context there
rg "write_some" "include/boost/beast2/server/http_stream.hpp" -B 10 -A 10 | head -60Repository: cppalliance/beast2
Length of output: 620
🏁 Script executed:
# Check if there are other instances in route_handler_corosio.hpp with the same pattern
rg "write_some" "include/boost/beast2/server/route_handler_corosio.hpp" -B 5 -A 8Repository: cppalliance/beast2
Length of output: 1307
Handle partial writes before consuming serializer buffers.
write_some may write fewer bytes than requested. The current code captures the return value n (bytes actually written) in lines 16, but ignores it and instead sums buffer sizes, then calls serializer.consume(written) with the full buffer size regardless of actual bytes sent. This causes data loss because the serializer skips more bytes than were actually transmitted, corrupting the response stream.
Compare with include/boost/beast2/server/http_stream.hpp, which correctly accumulates actual bytes written (written += n) before consuming. The fix requires retrying partial writes per buffer until all bytes are sent or an error occurs.
🐛 Proposed fix (write all bytes, consume actual count)
- auto bufs = *rv;
-
- // Write all buffers
- for(auto const& buf : bufs)
- {
- auto [ec, n] = co_await stream.write_some(
- capy::const_buffer(buf.data(), buf.size()));
- if(ec)
- co_return ec;
- }
-
- // Calculate total size written
- std::size_t written = 0;
- for(auto const& buf : bufs)
- written += buf.size();
+ auto bufs = *rv;
+ std::size_t written = 0;
+ for(auto const& buf : bufs)
+ {
+ std::size_t offset = 0;
+ auto const* data =
+ static_cast<char const*>(buf.data());
+ while(offset < buf.size())
+ {
+ auto [ec, n] = co_await stream.write_some(
+ capy::const_buffer(
+ data + offset,
+ buf.size() - offset));
+ if(ec)
+ co_return ec;
+ offset += n;
+ written += n;
+ }
+ }🤖 Prompt for AI Agents
In `@include/boost/beast2/server/route_handler_corosio.hpp` around lines 64 - 87,
The loop that writes serializer buffers uses stream.write_some inside
route_handler_corosio.hpp but ignores partial writes: it sums buf.size() and
calls serializer.consume(written) instead of the actual bytes sent, which can
drop data; update the write loop to handle partial writes per buffer by retrying
until each buffer's bytes are fully written (adjusting the buffer pointer/offset
passed to capy::const_buffer based on bytes written), accumulate the actual
bytes written into a variable (using the returned n from stream.write_some) and
call serializer.consume(actual_written) after sending all bytes or on error;
reference the serializer object, the for-loop over bufs, stream.write_some,
capy::const_buffer, and serializer.consume when making the change.
| // If using chunked encoding, send "0\r\n\r\n" | ||
| if(res.value_or( | ||
| http::field::transfer_encoding, "") == "chunked") | ||
| { | ||
| static constexpr char terminator[] = "0\r\n\r\n"; | ||
| auto [ec, n] = co_await stream.write_some( | ||
| capy::const_buffer(terminator, 5)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "route_handler_corosio.hpp" -type fRepository: cppalliance/beast2
Length of output: 117
🏁 Script executed:
fd -t f "route_handler_corosio.hpp"Repository: cppalliance/beast2
Length of output: 115
🏁 Script executed:
rg -l "Transfer-Encoding" --type cppRepository: cppalliance/beast2
Length of output: 44
🏁 Script executed:
cat -n include/boost/beast2/server/route_handler_corosio.hpp | sed -n '90,115p'Repository: cppalliance/beast2
Length of output: 974
🏁 Script executed:
rg -i "transfer.encoding" --type-listRepository: cppalliance/beast2
Length of output: 451
🏁 Script executed:
rg -i "chunked" include/boost/beast2/server/ -A 3 -B 3Repository: cppalliance/beast2
Length of output: 1262
🏁 Script executed:
rg -i "transfer.encoding" . --type hpp --type cppRepository: cppalliance/beast2
Length of output: 89
🌐 Web query:
RFC 7230 Transfer-Encoding header format multiple codings
💡 Result:
Short answer: Transfer-Encoding is a comma-separated (or repeated-header) ordered list of transfer-coding tokens; the codings are listed in the order they were applied (first applied first). The ABNF is Transfer-Encoding = 1#transfer-coding. The chunked coding, if used, MUST appear only once and if present as the final coding it determines framing; chunked MUST NOT be applied more than once. Also, when Transfer-Encoding is present it overrides Content-Length. [1][2]
References:
- RFC 7230 section 3.3.1 and section 4 (transfer-codings). [1]
- HTTPbis online rendering of RFC 7230 (same material). [2]
🌐 Web query:
RFC 7230 transfer-coding token case-sensitive
💡 Result:
Yes — RFC 7230 states that all transfer-coding names are case-insensitive (Section 4). [1][2]
Sources:
[1] RFC 7230 (rfc-editor), Section 4.
[2] RFC 7230 (IETF datatracker), Section 4.
🏁 Script executed:
find . -name "*.hpp" -o -name "*.h" | head -20Repository: cppalliance/beast2
Length of output: 838
🏁 Script executed:
rg "transfer.coding|transfer_coding" . --max-count 10Repository: cppalliance/beast2
Length of output: 44
🏁 Script executed:
rg "transfer_encoding" include/ -A 5 -B 5Repository: cppalliance/beast2
Length of output: 1119
🏁 Script executed:
rg "field::" include/boost/beast2/ -l | head -10Repository: cppalliance/beast2
Length of output: 115
🏁 Script executed:
find include/boost/beast2 -name "*.hpp" -type f | xargs grep -l "Transfer-Encoding\|transfer.encoding" 2>/dev/nullRepository: cppalliance/beast2
Length of output: 115
Transfer-Encoding "chunked" check must handle multiple codings and case-insensitivity.
RFC 7230 specifies that Transfer-Encoding is a comma-separated list of transfer-coding tokens (e.g., "gzip, chunked") and all token names are case-insensitive. The current exact-equality check will fail to send the terminating chunk ("0\r\n\r\n") in these cases, leaving clients hanging and violating the chunked encoding protocol. Parse the Transfer-Encoding header to check for the chunked token according to RFC 7230 rules.
🤖 Prompt for AI Agents
In `@include/boost/beast2/server/route_handler_corosio.hpp` around lines 99 - 105,
The Transfer-Encoding check currently uses exact equality on
res.value_or(http::field::transfer_encoding, "") which fails for comma-separated
or mixed-case tokens; update the condition to parse the header string returned
by res.value_or(...), split on commas, trim whitespace, and perform a
case-insensitive comparison of each token to "chunked" (so strings like "gzip,
chunked" or "Chunked" are detected). Replace the simple equality test near the
write_some call (the block using res and terminator) with this
tokenized/case-insensitive check and only send the "0\r\n\r\n" terminator when a
"chunked" token is present.
| write_impl(capy::buffer_param buffers) override | ||
| { | ||
| // Extract buffers from type-erased wrapper | ||
| constexpr std::size_t max_bufs = 16; | ||
| capy::mutable_buffer bufs[max_bufs]; | ||
| auto const n = buffers.copy_to(bufs, max_bufs); | ||
|
|
||
| // Write all buffers | ||
| for(std::size_t i = 0; i < n; ++i) | ||
| { | ||
| auto [ec, written] = co_await stream.write_some( | ||
| capy::const_buffer( | ||
| bufs[i].data(), | ||
| bufs[i].size())); | ||
| if(ec) | ||
| co_return ec; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's understand the file structure and locate relevant definitions
find . -type f -name "*.hpp" -o -name "*.h" | head -20Repository: cppalliance/beast2
Length of output: 838
🏁 Script executed:
# Search for buffer_param definition
rg "class buffer_param|struct buffer_param" --type hppRepository: cppalliance/beast2
Length of output: 89
🏁 Script executed:
# Search for write_some implementation and usage patterns
rg "write_some" --type hpp -B 2 -A 2 | head -50Repository: cppalliance/beast2
Length of output: 89
🏁 Script executed:
# Look at the broader context of the write_impl function
cat include/boost/beast2/server/route_handler_corosio.hpp | head -150Repository: cppalliance/beast2
Length of output: 3899
🏁 Script executed:
# Search for capy namespace definitions
rg "namespace capy" --type hpp -A 5Repository: cppalliance/beast2
Length of output: 89
🏁 Script executed:
# Look for copy_to implementation
rg "copy_to\(" --type hpp -B 2 -A 5Repository: cppalliance/beast2
Length of output: 89
🏁 Script executed:
# Search for buffer_param definition without file type
rg "class buffer_param|struct buffer_param" -A 10Repository: cppalliance/beast2
Length of output: 44
🏁 Script executed:
# Search for capy includes and namespace
rg "namespace capy" -B 2 -A 5Repository: cppalliance/beast2
Length of output: 2890
🏁 Script executed:
# Look at the send() method which seems to handle buffers properly for comparison
sed -n '50,105p' include/boost/beast2/server/route_handler_corosio.hppRepository: cppalliance/beast2
Length of output: 1784
🏁 Script executed:
# Search for write_some in the codebase to understand its behavior
rg "write_some" -B 3 -A 3Repository: cppalliance/beast2
Length of output: 6914
🏁 Script executed:
# Look for buffer_param.hpp file
find . -name "*buffer_param*" -type fRepository: cppalliance/beast2
Length of output: 44
🏁 Script executed:
# Look at http_stream.hpp which also uses write_some to compare patterns
cat include/boost/beast2/server/http_stream.hppRepository: cppalliance/beast2
Length of output: 10046
🏁 Script executed:
# Search for corosio socket or write_some documentation
rg "corosio" -A 5 -B 2 | head -100Repository: cppalliance/beast2
Length of output: 4328
🏁 Script executed:
# Look for tests that might show expected behavior
find . -name "*test*" -type f | grep -E "(corosio|route_handler)" | head -10Repository: cppalliance/beast2
Length of output: 44
🏁 Script executed:
# Check if buffer_param is documented in any includes
rg "buffer_param" -B 2 -A 5 | head -60Repository: cppalliance/beast2
Length of output: 1410
write_impl silently truncates buffer sequences and does not handle partial writes.
The copy_to(bufs, max_bufs=16) call will drop any buffers beyond 16 without indication, causing data loss. Additionally, write_some may return fewer bytes written than requested, but the code ignores the written value and proceeds to the next buffer without retrying, causing further data loss on partial writes.
🐛 Proposed fix
- http::route_task
- write_impl(capy::buffer_param buffers) override
- {
- // Extract buffers from type-erased wrapper
- constexpr std::size_t max_bufs = 16;
- capy::mutable_buffer bufs[max_bufs];
- auto const n = buffers.copy_to(bufs, max_bufs);
-
- // Write all buffers
- for(std::size_t i = 0; i < n; ++i)
- {
- auto [ec, written] = co_await stream.write_some(
- capy::const_buffer(
- bufs[i].data(),
- bufs[i].size()));
- if(ec)
- co_return ec;
- }
-
- co_return {};
- }
+ http::route_task
+ write_impl(capy::buffer_param buffers) override
+ {
+ // Extract buffers from type-erased wrapper
+ std::size_t max_bufs = 16;
+ capy::mutable_buffer bufs[16];
+ auto n = buffers.copy_to(bufs, max_bufs);
+
+ std::size_t buf_index = 0;
+ while(buf_index < n)
+ {
+ // If we hit the 16-buffer limit, extract more
+ if(buf_index >= max_bufs && buffers.buffer_count() > n)
+ {
+ n += buffers.copy_to(bufs, max_bufs, n);
+ }
+
+ std::size_t offset = 0;
+ auto const* data = static_cast<char const*>(bufs[buf_index].data());
+ while(offset < bufs[buf_index].size())
+ {
+ auto [ec, written] = co_await stream.write_some(
+ capy::const_buffer(
+ data + offset,
+ bufs[buf_index].size() - offset));
+ if(ec)
+ co_return ec;
+ offset += written;
+ }
+ ++buf_index;
+ }
+
+ co_return {};
+ }🤖 Prompt for AI Agents
In `@include/boost/beast2/server/route_handler_corosio.hpp` around lines 115 -
131, write_impl currently truncates if buffers.copy_to(bufs, max_bufs) exceeds
max_bufs and ignores partial writes from stream.write_some; fix by processing
the full buffer_sequence in chunks: repeatedly copy up to max_bufs via
buffers.copy_to(...) (using an offset/iterator or repeatedly removing copied
prefix) and for each capy::mutable_buffer attempt to write until its bytes are
consumed by looping on stream.write_some and advancing the buffer by the
returned written count on partial writes, retrying on transient errors and
returning ec on fatal errors; update references in write_impl to use
buffers.copy_to, capy::mutable_buffer bufs[], and stream.write_some when
implementing this chunked+retry logic.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@CMakePresets.json`:
- Around line 2-7: CMakePresets.json currently sets cmakeMinimumRequired to
3.25.0 which conflicts with the project's CMakeLists.txt that declares
cmake_minimum_required(VERSION 3.8...3.20); fix by making these consistent:
either lower CMakePresets.json cmakeMinimumRequired to 3.8.0 to match the
existing CMakeLists.txt, or update the CMakeLists.txt cmake_minimum_required
declaration to 3.25.0 (and adjust any code requiring newer CMake features) so
both CMakePresets.json and the cmake_minimum_required(VERSION ...) in
CMakeLists.txt reference the same minimum version.
| "version": 6, | ||
| "cmakeMinimumRequired": { | ||
| "major": 3, | ||
| "minor": 25, | ||
| "patch": 0 | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check for cmake_minimum_required declarations
rg -n 'cmake_minimum_required' --glob 'CMakeLists.txt'
# Check CI/CD configurations
rg -n 'cmake' --glob '*.yml' --glob '*.yaml' -i
# Check README documentation
rg -n 'cmake' --glob '*README*' -i
# Also check if there are any other CMake configuration files
fd -e 'CMakeLists.txt' -o -e 'CMakePresets.json'Repository: cppalliance/beast2
Length of output: 450
CMakePresets.json requires CMake 3.25.0 but CMakeLists.txt only supports up to 3.20.
The schema version 6 and cmakeMinimumRequired 3.25.0 in CMakePresets.json create an incompatibility: the main CMakeLists.txt declares cmake_minimum_required(VERSION 3.8...3.20), which allows CMake up to 3.20. Users with CMake 3.21–3.24 will be unable to use presets while the main build remains compatible.
Either update CMakeLists.txt to require CMake 3.25.0, or lower CMakePresets.json's cmakeMinimumRequired to match the actual project minimum (3.8).
🤖 Prompt for AI Agents
In `@CMakePresets.json` around lines 2 - 7, CMakePresets.json currently sets
cmakeMinimumRequired to 3.25.0 which conflicts with the project's CMakeLists.txt
that declares cmake_minimum_required(VERSION 3.8...3.20); fix by making these
consistent: either lower CMakePresets.json cmakeMinimumRequired to 3.8.0 to
match the existing CMakeLists.txt, or update the CMakeLists.txt
cmake_minimum_required declaration to 3.25.0 (and adjust any code requiring
newer CMake features) so both CMakePresets.json and the
cmake_minimum_required(VERSION ...) in CMakeLists.txt reference the same minimum
version.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.