Skip to content

Find ReadUntil delimiter split between reads - #142

Merged
resetius merged 4 commits into
masterfrom
readuntil-split-delimiter
Nov 4, 2026
Merged

resetius merged 4 commits into
masterfrom
readuntil-split-delimiter

Conversation

@resetius

@resetius resetius commented Nov 4, 2026 •

Copy link
Copy Markdown
Owner

Problem

TByteReader::ReadUntil searched for the delimiter only in the internal buffer and, before every new read, moved the whole buffer into the result. A delimiter split between two reads (TCP segmentation, or crossing the 1024-byte boundary of the temporary read buffer) was therefore never found, and the coroutine kept waiting for data that had already arrived. The HTTP server reads request headers with ReadUntil("\r\n\r\n") and chunk headers with ReadUntil("\r\n"), so such requests hung until the peer (e.g. nginx) gave up.

Fix

Data stays in the buffer and the search resumes from size - (delimiter.size() - 1) after each read, so a split delimiter is found without rescanning the accumulated data. The returned value is unchanged (everything up to and including the delimiter); bytes after it stay buffered. An empty delimiter returns an empty string (documented).

Other readers checked: TLineReader, TLineSplitter, TZeroCopyLineSplitter (single-byte delimiter, cannot be split), ws.hpp (only calls ReadUntil), httpd (only calls ReadUntil); no other occurrence of the pattern.

Tests

In tests/test_http.cpp: delimiter split at every position, one byte per read, delimiter crossing the 1024-byte boundary, two consecutive delimiters, and an end-to-end TWebServer request whose header is split between two writes. All of them fail on master (missed delimiter reported as closed connection, or a 5 s deadline in the server test) and pass with the fix.

The free-port helper used by the tests is now NNet::GetFreePort() in coroio/address.hpp (it replaces getport() in tests/tests.cpp and is used by the new server test).

@github-actions

github-actions Bot commented Nov 4, 2026 •

Copy link
Copy Markdown

Unit Test Results (Linux)

179 tests  +6   179 ✅ +6   15s ⏱️ +2s
  4 suites ±0     0 💤 ±0 
  4 files   ±0     0 ❌ ±0 

Results for commit e40e8f3. ± Comparison against base commit 45c931a.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Nov 4, 2026 •

Copy link
Copy Markdown

Unit Test Results (MacOS)

146 tests  +6   146 ✅ +6   23s ⏱️ ±0s
  4 suites ±0     0 💤 ±0 
  4 files   ±0     0 ❌ ±0 

Results for commit e40e8f3. ± Comparison against base commit 45c931a.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Nov 4, 2026 •

Copy link
Copy Markdown

Unit Test Results (Windows)

134 tests  +5   134 ✅ +5   39s ⏱️ ±0s
  4 suites ±0     0 💤 ±0 
  4 files   ±0     0 ❌ ±0 

Results for commit a36d9db. ± Comparison against base commit 45c931a.

♻️ This comment has been updated with latest results.

@resetius

resetius commented Nov 4, 2026

Copy link
Copy Markdown
Owner Author

Independent review of head a36d9db751923dbe2069e221c830985e5eeb27a3. Verdict: no P1/P2 findings, OK to merge; one P3 in the new empty-delimiter claim and one unrelated P3 side finding.

Checked:

  • ASan Debug build on macOS: ut_tests (106) and ut_test_http (12, run 5 times) pass. CI on this head: Linux, macOS, Windows builds and tests pass.
  • Resume offset: a delimiter of length d that starts at index s was checkable once s + d <= size, so the first unchecked start is size - d + 1 = size - (d - 1), which is exactly the new scanned; size < d gives 0. Verified by the tests (every two-chunk split of a header, byte by byte, every offset around the 1024-byte read, two delimiters in one buffer with the second split) and by reading the code.
  • What stays in Buffer: only the bytes after the matched delimiter; the result is built once from [0, size). The old code copied every non-matching chunk into result and lost the split delimiter, which was the production hang; the new test_http_server_header_split_between_writes is exactly that scenario (header split by 50 ms) and would hang to its 5 s deadline without the fix.
  • Hot path: no rescans (at most d-1 bytes re-examined per read), no maps, no new allocations beyond the single std::string result that was already there; the front erase on the deque removes a prefix, which is O(erased bytes) and happens once per call. A header that is already fully buffered (pipelining) is found in the first search.
  • Long headers: still linear (1024 bytes per read appended, searched once); there is no cap on header size, same as before.
  • GetFreePort(): namespace NNet, public, Doxygen present and honest about the port not being reserved; identical logic to the already merged test helper; the 21 call sites in tests.cpp and the copy in test_http.cpp are replaced mechanically.
  • LSan: __lsan::ScopedDisabler only wraps the call that allocates the detached accept-loop frame and only on Linux ASan; it does not hide other allocations. TWebServer has no way to stop Start() (infinite accept loop, frame is never destroyed), so the suppression is the pragmatic choice for a test; a Stop() or a future owned by the server would remove the need.

P3:

  • coroio/sockutils.hpp:~183 (ReadUntil), empty delimiter: the doc and the description say it "returns an empty string", but with an empty Buffer it does not. std::search with an empty pattern returns begin(), which equals Buffer.end() on an empty buffer, so the "found" test fails, scanned underflows (delimiter.size() - 1 wraps) and after the first read the search starts at index 1. Repro (chunk "abc", reader.ReadUntil("")): returns "a" and leaves "bc" buffered; with a non-empty buffer it does return "". Not used with an empty delimiter in the library, but either drop the claim or add if (delimiter.empty()) co_return {}; at the top and a test.
  • coroio/address.hpp GetFreePort(): on Windows it needs Winsock to be initialised (TInitializer) before the call; worth one doc sentence. It also only probes IPv4 loopback (stated in the doc).
  • Side finding, not touched by this PR (coroio/http/httpd.cpp:368 and httpd.hpp docs of WriteBodyFull): the doc says "Sends headers (with Content-Length) and the full body in one call. Implies SendHeaders()", but WriteBodyFull only writes the body. Repro: router that does SetStatus(200); co_await response.WriteBodyFull("hello");, raw client reads hello and nothing else, no status line, no headers (client got [hello]). Callers must call SendHeaders() first (qumir-service does); fix the doc or implement the implicit SendHeaders() and Content-Length.

@resetius resetius added the agent-reviewed An independent review agent checked this PR; the verdict is in a PR comment label Nov 4, 2026
@resetius
resetius merged commit cae2aef into master Nov 4, 2026
6 checks passed
@resetius
resetius deleted the readuntil-split-delimiter branch November 4, 2026 01:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-reviewed An independent review agent checked this PR; the verdict is in a PR comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant