Several fixes around Chunk processing (Json) of a large Json document and available length passed to fill send buffer #349
Conversation
WalkthroughThe pull request refactors byte tracking in async response classes and the ChunkPrint helper. Three JSON response methods now return actual bytes written rather than requested length, ChunkPrint fields are renamed to better reflect position semantics, and content-length constraints are properly enforced during reads. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/WebResponses.cpp (1)
472-485: Clamp by remaining content length only when Content‑Length is knownThe new
std::min(..., _contentLength - _sentLength)correctly prevents over‑reading when a Content‑Length is set, but it also forceslen == 0for responses where_sendContentLength == falseand_contentLengthis left at 0 (e.g.,AsyncCallbackResponseconstructed withlen == 0to stream until the filler returns 0). In that case_fillBufferAndProcessTemplatesis always called withlen == 0, so callbacks that previously streamed data now send nothing, and_contentLength - _sentLengthcan also underflow if_sentLengthever exceeds_contentLength.To preserve existing behavior for unknown‑length responses while still fixing the Content‑Length bug, consider something like:
- } else { - size_t const readLen = _fillBufferAndProcessTemplates(_send_buffer->data(), std::min(std::min(_send_buffer->size(), tcp_win), _contentLength - _sentLength)); + } else { + size_t maxLen = std::min(_send_buffer->size(), tcp_win); + if (_sendContentLength && _sentLength < _contentLength) { + maxLen = std::min(maxLen, _contentLength - _sentLength); + } + size_t const readLen = _fillBufferAndProcessTemplates(_send_buffer->data(), maxLen);This keeps the new “respect remaining Content‑Length” behavior for known‑length responses, avoids size_t underflow, and leaves the
len == 0/no‑Content‑Length streaming callbacks working as before.
🧹 Nitpick comments (1)
src/ChunkPrint.h (1)
11-23: ChunkPrint state and inline constructor look correct;written()could beconstThe new
_from/_len/_indexfields and inline constructor align with the updatedwrite(uint8_t)logic, and exposingwritten()is exactly what the JSON/MsgPack paths need. As a small style/readability tweak, you could markwritten()asconstsince it doesn’t mutate state:- size_t written() { - return _index; - } + size_t written() const { + return _index; + }Purely optional; behavior is fine as is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/AsyncJson.cpp(3 hunks)src/ChunkPrint.cpp(1 hunks)src/ChunkPrint.h(1 hunks)src/WebResponses.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/ChunkPrint.h (1)
src/ChunkPrint.cpp (2)
write(6-23)write(6-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: RPI (arduino-cli) (rpipico2w)
- GitHub Check: RPI (arduino-cli) (rpipicow)
- GitHub Check: ESP8266 (pio) (d1_mini)
- GitHub Check: ESP8266 (pio) (huzzah)
- GitHub Check: ESP8266 (arduino-cli)
- GitHub Check: ESP32 (pio) - Arduino 2 (esp32-s3-devkitc-1)
- GitHub Check: ESP32 (pio) - Arduino 3 (esp32-s3-devkitc-1)
- GitHub Check: ESP32 (pio) - Arduino 3 (esp32-s2-saola-1)
- GitHub Check: ESP32 (pio) - Specific Envs (ci-no-json)
- GitHub Check: ESP32 (pio) - Specific Envs (ci-arduino-3-esp-idf-log)
- GitHub Check: ESP32 (pio) - Arduino 3 (esp32-p4)
- GitHub Check: ESP32 (pio) - Specific Envs (ci-latest-asynctcp)
- GitHub Check: ESP32 (pio) - Specific Envs (ci-regex)
- GitHub Check: ESP32 (pio) - Specific Envs (ci-arduino-2-esp-idf-log)
- GitHub Check: ESP32 (pio) - Arduino 3 (esp32dev)
- GitHub Check: ESP32 (pio) - Specific Envs (ci-no-chunk-inflight)
- GitHub Check: ESP32 (pio) - Arduino 2 (esp32dev)
- GitHub Check: ESP32 (pio) - Arduino 2 (esp32-s2-saola-1)
- GitHub Check: ESP32 (arduino-cli) - Dev
- GitHub Check: ESP32 (arduino-cli) - Release
🔇 Additional comments (2)
src/AsyncJson.cpp (1)
57-65: Returningdest.written()correctly reports actual bytes producedSwitching the JSON/pretty‑JSON/MessagePack
_fillBuffermethods to returndest.written()instead of the requestedlenfixes over‑reporting near the end of the payload and keeps_sentLengthaligned with the bytes actually serialized into the buffer. This matches the new ChunkPrint semantics and is the right behavior for the streaming response machinery.Also applies to: 85-93, 106-110
src/ChunkPrint.cpp (1)
6-22: ChunkPrint::write(uint8_t) correctly enforces skip and length windowThe new implementation cleanly skips the first
_frombytes, then writes at most_lenbytes into_destination, and finally returns 0 once the window is exhausted, which is exactly what the JSON/MsgPack chunking logic expects. The early_len == 0bailout also avoids unnecessary work for zero‑length calls; no changes needed here.
4ff1443 to
b2e5f15
Compare
The calculated len was not taking in consideration the remaining bytes to send when a content-length was set.
f0b308e to
19a957c
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes two bugs related to buffer length handling in the web server response system:
- Corrects the length calculation in
write_send_buffsto account for remaining content when content-length is set - Updates
ChunkPrintto return actual bytes written instead of buffer capacity
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/WebResponses.cpp | Adds _contentLength - _sentLength constraint to prevent reading beyond remaining content in non-chunked responses |
| src/ChunkPrint.h | Refactors member variables for clarity, moves constructor inline, and adds written() method to return actual bytes written |
| src/ChunkPrint.cpp | Updates write() implementation with clearer logic and comments for byte skipping and writing |
| src/AsyncJson.cpp | Changes three methods to return dest.written() instead of len to report actual bytes written rather than buffer size |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@me-no-dev @vortigont @willmmiles : please have a look at these bug fixes when you can: serving json content is not correctly working for big documents currently. |
|
Just a quick view while I'm on travel this looks logically wrong to me |
That was my first thought too, actually! So I went back and checked older versions. It turns out that limiting the length passed to the callback has been the behaviour for over a decade. I'm not sure I'd do it that way on a clean sheet design, but for a library like this, it's probably better to keep the behaviour constant. |
Yes right now the idea is to merge this fix asap and release until a more correct approach is done. There were 2 issues that are corrected here:
Changing the behavior and sending a differently calculated len to the callbacks may also impacts other parts of the code, not only the json implementation where the bug was seen. So if you change the behavior, you also need to review the template code to make sure it remains valid. |
See: #349 Fix bug for non-chucked responses without a known content-length (see LargeResponse example)
URGENT - Fix bug introduced in #349
Fix length passed to callback
The calculated len was not taking in consideration the remaining bytes to send when a content-length was set.
Fix ChunkPrint so that it returns written bytes and not the available bytes to write
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.