Keep the error body of a failed streaming request, and fix the three framing defects the review found - #14
Merged
Conversation
send() fills HttpResponse::body on every path including failures; send_stream() was the one entry point that dropped it. A non-2xx answer to a streaming request is an error document, not an event stream: SseParser finds no event boundary in it, emits nothing, and the bytes stay in its private buffer. Callers were left with a status line and no reason. Capture the body when the status is not 2xx. Events are still parsed and dispatched exactly as before, and nothing is copied on a 2xx stream, so the success path is byte-identical. The copy is bounded by stream_error_body_limit (1 MiB) so a server answering 5xx with an endless body cannot grow the buffer without limit. Truncation lives in an exported append_within_limit, in the same spirit as parse_chunk_size_line, with three unit tests for under, across and past the limit; a live test against httpbin's /status/418 covers the wiring.
…ize rather than salvaging it
Review of the change this branch already carries. The defect it reports is real
and the fix is placed correctly --- `dispatch` is the single funnel for every
body byte on both framing paths, and `captureBody` is decided after the headers
are read, where the status is final. Measured against master, one program, one
source file:
master status=418 events=0 body.size()=0
this status=418 events=0 body.size()=135
What follows is what that fix could not do on its own.
--- 1. A DECLARED LENGTH, WHICH IS WHY THE TEST HAD TO CLOSE THE CONNECTION ----
`send_stream` had no branch for `Content-Length`: a response that was not
chunked was read until the connection closed, whatever its headers said. On this
library's own defaults --- `keepAlive = true`, so the request carries
`Connection: keep-alive` --- the server does not close, and the read loop ran
until `readTimeoutMs` expired. Measured against httpbin's `/status/418`:
keepAlive = false status=418 body=135 elapsed 1370 ms
keepAlive = true status=418 body=135 elapsed 9379 ms (timeout 8000)
The error body arrived either way, and on the defaults it arrived a full read
timeout late --- sixty seconds, as the defaults stand. `send()` has had this
branch throughout, which is the same asymmetry between the two entry points that
this branch exists to remove.
The live test set `keepAlive = false`, "so the server closes and the read loop
ends". That comment was the defect, and the test was examining the one
arrangement in which it does not appear. It now runs on the defaults and asserts
the elapsed time.
after: keepAlive = true status=418 body=135 elapsed 1192 ms
--- 2. A CHUNK SIZE THAT DOES NOT PARSE IS NOT A TERMINAL CHUNK ---------------
`parse_hex` returns what it accumulated when it meets a character it does not
recognise, and zero for an empty line --- and `read_line` returns an empty line
on a timeout or a closed connection. So a stream that was cut short read as a
stream that ended cleanly and this loop reported success. #9 established
`parse_chunk_size_line` for exactly this and it reached `download_to_file`
alone; `send` and `send_stream` were left on the old one.
--- 3. Content-Length WAS PARSED BY KEEPING THE DIGITS ------------------------
Measured, by compiling that parser on its own:
"135" -> 135
"abc" -> 0 <- a refusal read as a real zero
"12abc" -> 12 <- stops twelve bytes in
"-1" -> 1 <- the sign is discarded
"99999999999999999999" -> 7766279631452241919 <- wraps, in silence
The last two are the ones no care at the call site could recover from, because
what it receives is a plausible number. `parse_content_length` is exported and
shaped like `parse_chunk_size_line`, for the reason #9 gave: it is the half of
the body framing that can be examined without a server. Both readers use it.
--- criteria -----------------------------------------------------------------
Six unit tests over the two pure parsers, and two live ones: the failed stream
now runs on the DEFAULT configuration with the elapsed time asserted, and a
chunked 2xx is asserted to leave `body` empty and to return promptly --- the
success path is the one this change restructured around, so it is observed
rather than assumed.
17 tests from 6 suites pass, plus 3 in test_resolver.
Member
Author
|
|
This was referenced Aug 29, 2026
Sunrisepeak
added a commit
that referenced
this pull request
Sep 5, 2026
…ts host Closes #15 and #16. Version 0.3.0. #16 first, because it has to be. A write to a socket whose peer has gone away raises SIGPIPE, and a program that has not disarmed it — the default — is killed rather than told. The fd is one this library created and the write is usually the close_notify its own pool clean-up sends, so this is the library's defect. mbedtls guards against it in net_prepare with a process-wide signal(SIGPIPE, SIG_IGN); replacing mbedtls's network layer with a custom BIO dropped that guard and put nothing in its place. MSG_NOSIGNAL, and SO_NOSIGPIPE where that does not exist, is the better replacement anyway: a library has no business changing its host's signal disposition, and a program that wants SIGPIPE on its own stdout still gets it. Which of the two applies is decided by the target's own <sys/socket.h> and by nothing else — measured: glibc and musl carry MSG_NOSIGNAL and not SO_NOSIGPIPE, Darwin the reverse, Windows neither and no signal to raise. P0 had to land before everything else here, because everything else makes the drop path more common. Then #15. It named two of the eleven paths that could leak a connection; all eleven are fixed. Four of the other nine turned up while verifying the report — a redirect, a non-2xx and a failed file open, none of which involve a timeout or a truncation at all, plus a Content-Length past 32 bits — and two more were regressions 0.2.10 had introduced into the streaming reader. Every one of the eleven has the same shape: an early-return path that did nothing, where doing nothing left a socket with unread bytes in the pool for the next request to pick up. So dropping is now what doing nothing means — a PooledConnection guard whose destructor drops, and one call to keep() on the single path where the body was read to the end its framing declared. The reason there were eleven rather than one is that send, send_stream and download_to_file each carried a near-copy of the status-line parse, the header loop and the body loop, and every past hardening had landed on one or two of the three. #14 is the most recent example: it added a Content-Length branch to one copy and introduced two regressions doing it. There is now one status-line parser, one header reader and one body reader. read_body returns where the body ended, and that is the same question as whether the connection can be reused. Two further unbounded loops found while writing that reader, neither reported: a header block accumulates into a map, so an endless supply of short, well formed header lines grows the client's memory without limit; a trailer section is discarded but holds the call open just as long, because every line resets the read timeout. Both are bounded now, as each line's length already was. Added, all of it additive: HttpResponse::bodyComplete and bodyError, because a truncated 200 and a complete 200 were indistinguishable to the caller; ok() does not consult them, so existing code means what it did. maxResponseBodyBytes, because the size send() was about to allocate came from a header. retryOnStaleConnection, because a server closing an idle keep-alive connection is routine and the client cannot see it until it writes — that failure reached callers as "No response" for a request the server never saw. The window is one attempt, on a pooled connection, before a single response byte has arrived. tests/ had no keep-alive coverage at all, which is why #14's regression merged green. It now scripts an in-process TLS server — the library speaks only HTTPS, so a plain listener cannot reach the code under test — and every pool test asserts how many TCP connections the server saw as well as what came back. The report for #15 contains a case whose output is byte-for-byte correct and whose only symptom is the connection count. Each fix was checked by mutation. Three tests did not survive that and were rewritten: the SIGPIPE child stopped at its first failed write, which returns ECONNRESET without a signal; and the pool servers stalled rather than sending their remaining bytes, so the connection was silent rather than dirty and the stale-connection retry rescued the second request whether or not the guard worked. Also here, because all three are about the same claim — that this library works where it says it does: examples/openkal builds these sources above openkal, the portable kernel ABI, and makes a real HTTPS request through kal_net_connect. It is a separate CI job because whether MSG_NOSIGNAL or SO_NOSIGPIPE exists is decided by the C library rather than the operating system, and a #ifdef that is wrong about that compiles cleanly on the gcc job and fails there — which is how 5e7d66f reached master. templates/ ships three starting points for `mcpp new --template tinyhttps`, one per entry point. tools/template_smoke.sh renders and builds them against the working tree, because `mcpp new` can only reach a template that is already published, and checking them after the release makes the first user the one who finds out. It has already caught one: import std carries no stdout macro, so a progress bar flushed through it compiled here and not in a generated project. mcpp.lock is removed and ignored. The file's own header says it does not pin anything — "index dependencies are re-resolved from their constraints each time" — so for a library, whose consumers resolve from its constraints, it recorded one machine's resolution and changed nothing about anyone else's. Eight of the ten mcpplibs packages already ignore it, the scaffold template among them.
Sunrisepeak
added a commit
that referenced
this pull request
Sep 5, 2026
…ts host Closes #15 and #16. Version 0.3.0. #16 first, because it has to be. A write to a socket whose peer has gone away raises SIGPIPE, and a program that has not disarmed it — the default — is killed rather than told. The fd is one this library created and the write is usually the close_notify its own pool clean-up sends, so this is the library's defect. mbedtls guards against it in net_prepare with a process-wide signal(SIGPIPE, SIG_IGN); replacing mbedtls's network layer with a custom BIO dropped that guard and put nothing in its place. MSG_NOSIGNAL, and SO_NOSIGPIPE where that does not exist, is the better replacement anyway: a library has no business changing its host's signal disposition, and a program that wants SIGPIPE on its own stdout still gets it. Which of the two applies is decided by the target's own <sys/socket.h> and by nothing else — measured: glibc and musl carry MSG_NOSIGNAL and not SO_NOSIGPIPE, Darwin the reverse, Windows neither and no signal to raise. P0 had to land before everything else here, because everything else makes the drop path more common. Then #15. It named two of the eleven paths that could leak a connection; all eleven are fixed. Four of the other nine turned up while verifying the report — a redirect, a non-2xx and a failed file open, none of which involve a timeout or a truncation at all, plus a Content-Length past 32 bits — and two more were regressions 0.2.10 had introduced into the streaming reader. Every one of the eleven has the same shape: an early-return path that did nothing, where doing nothing left a socket with unread bytes in the pool for the next request to pick up. So dropping is now what doing nothing means — a PooledConnection guard whose destructor drops, and one call to keep() on the single path where the body was read to the end its framing declared. The reason there were eleven rather than one is that send, send_stream and download_to_file each carried a near-copy of the status-line parse, the header loop and the body loop, and every past hardening had landed on one or two of the three. #14 is the most recent example: it added a Content-Length branch to one copy and introduced two regressions doing it. There is now one status-line parser, one header reader and one body reader. read_body returns where the body ended, and that is the same question as whether the connection can be reused. Two further unbounded loops found while writing that reader, neither reported: a header block accumulates into a map, so an endless supply of short, well formed header lines grows the client's memory without limit; a trailer section is discarded but holds the call open just as long, because every line resets the read timeout. Both are bounded now, as each line's length already was. Added, all of it additive: HttpResponse::bodyComplete and bodyError, because a truncated 200 and a complete 200 were indistinguishable to the caller; ok() does not consult them, so existing code means what it did. maxResponseBodyBytes, because the size send() was about to allocate came from a header. retryOnStaleConnection, because a server closing an idle keep-alive connection is routine and the client cannot see it until it writes — that failure reached callers as "No response" for a request the server never saw. The window is one attempt, on a pooled connection, before a single response byte has arrived. tests/ had no keep-alive coverage at all, which is why #14's regression merged green. It now scripts an in-process TLS server — the library speaks only HTTPS, so a plain listener cannot reach the code under test — and every pool test asserts how many TCP connections the server saw as well as what came back. The report for #15 contains a case whose output is byte-for-byte correct and whose only symptom is the connection count. Each fix was checked by mutation. Three tests did not survive that and were rewritten: the SIGPIPE child stopped at its first failed write, which returns ECONNRESET without a signal; and the pool servers stalled rather than sending their remaining bytes, so the connection was silent rather than dirty and the stale-connection retry rescued the second request whether or not the guard worked. Two more found by review after the above was written, one of them introduced by it. Reading past a 1xx is new: a 103 Early Hints or a 100 Continue was returned to the caller as the answer, and — once a 1xx counted as a response with no body — the connection was marked clean with the real response still sitting on it, which is #15 arriving by another door. And the BIO answered a peer's FIN with MBEDTLS_ERR_NET_CONN_RESET where mbedtls's own passes the zero through, so `ssl_fetch_input`'s test for exactly that zero (ssl_msg.c:2251) never fired; a body whose framing IS the close then read as truncated, and download_to_file reported ok() == false for a file that had arrived complete. Also here, because all three are about the same claim — that this library works where it says it does: examples/openkal builds these sources above openkal, the portable kernel ABI, and makes a real HTTPS request through kal_net_connect. It is a separate CI job because whether MSG_NOSIGNAL or SO_NOSIGPIPE exists is decided by the C library rather than the operating system, and a #ifdef that is wrong about that compiles cleanly on the gcc job and fails there — which is how 5e7d66f reached master. templates/ ships three starting points for `mcpp new --template tinyhttps`, one per entry point. tools/template_smoke.sh renders and builds them against the working tree, because `mcpp new` can only reach a template that is already published, and checking them after the release makes the first user the one who finds out. It has already caught one: import std carries no stdout macro, so a progress bar flushed through it compiled here and not in a generated project. mcpp.lock is removed and ignored. The file's own header says it does not pin anything — "index dependencies are re-resolved from their constraints each time" — so for a library, whose consumers resolve from its constraints, it recorded one machine's resolution and changed nothing about anyone else's. Eight of the ten mcpplibs packages already ignore it, the scaffold template among them.
Sunrisepeak
added a commit
that referenced
this pull request
Sep 5, 2026
…ts host (#17) Closes #15 and #16. Version 0.3.0. #16 first, because it has to be. A write to a socket whose peer has gone away raises SIGPIPE, and a program that has not disarmed it — the default — is killed rather than told. The fd is one this library created and the write is usually the close_notify its own pool clean-up sends, so this is the library's defect. mbedtls guards against it in net_prepare with a process-wide signal(SIGPIPE, SIG_IGN); replacing mbedtls's network layer with a custom BIO dropped that guard and put nothing in its place. MSG_NOSIGNAL, and SO_NOSIGPIPE where that does not exist, is the better replacement anyway: a library has no business changing its host's signal disposition, and a program that wants SIGPIPE on its own stdout still gets it. Which of the two applies is decided by the target's own <sys/socket.h> and by nothing else — measured: glibc and musl carry MSG_NOSIGNAL and not SO_NOSIGPIPE, Darwin the reverse, Windows neither and no signal to raise. P0 had to land before everything else here, because everything else makes the drop path more common. Then #15. It named two of the eleven paths that could leak a connection; all eleven are fixed. Four of the other nine turned up while verifying the report — a redirect, a non-2xx and a failed file open, none of which involve a timeout or a truncation at all, plus a Content-Length past 32 bits — and two more were regressions 0.2.10 had introduced into the streaming reader. Every one of the eleven has the same shape: an early-return path that did nothing, where doing nothing left a socket with unread bytes in the pool for the next request to pick up. So dropping is now what doing nothing means — a PooledConnection guard whose destructor drops, and one call to keep() on the single path where the body was read to the end its framing declared. The reason there were eleven rather than one is that send, send_stream and download_to_file each carried a near-copy of the status-line parse, the header loop and the body loop, and every past hardening had landed on one or two of the three. #14 is the most recent example: it added a Content-Length branch to one copy and introduced two regressions doing it. There is now one status-line parser, one header reader and one body reader. read_body returns where the body ended, and that is the same question as whether the connection can be reused. Two further unbounded loops found while writing that reader, neither reported: a header block accumulates into a map, so an endless supply of short, well formed header lines grows the client's memory without limit; a trailer section is discarded but holds the call open just as long, because every line resets the read timeout. Both are bounded now, as each line's length already was. Added, all of it additive: HttpResponse::bodyComplete and bodyError, because a truncated 200 and a complete 200 were indistinguishable to the caller; ok() does not consult them, so existing code means what it did. maxResponseBodyBytes, because the size send() was about to allocate came from a header. retryOnStaleConnection, because a server closing an idle keep-alive connection is routine and the client cannot see it until it writes — that failure reached callers as "No response" for a request the server never saw. The window is one attempt, on a pooled connection, before a single response byte has arrived. tests/ had no keep-alive coverage at all, which is why #14's regression merged green. It now scripts an in-process TLS server — the library speaks only HTTPS, so a plain listener cannot reach the code under test — and every pool test asserts how many TCP connections the server saw as well as what came back. The report for #15 contains a case whose output is byte-for-byte correct and whose only symptom is the connection count. Each fix was checked by mutation. Three tests did not survive that and were rewritten: the SIGPIPE child stopped at its first failed write, which returns ECONNRESET without a signal; and the pool servers stalled rather than sending their remaining bytes, so the connection was silent rather than dirty and the stale-connection retry rescued the second request whether or not the guard worked. Two more found by review after the above was written, one of them introduced by it. Reading past a 1xx is new: a 103 Early Hints or a 100 Continue was returned to the caller as the answer, and — once a 1xx counted as a response with no body — the connection was marked clean with the real response still sitting on it, which is #15 arriving by another door. And the BIO answered a peer's FIN with MBEDTLS_ERR_NET_CONN_RESET where mbedtls's own passes the zero through, so `ssl_fetch_input`'s test for exactly that zero (ssl_msg.c:2251) never fired; a body whose framing IS the close then read as truncated, and download_to_file reported ok() == false for a file that had arrived complete. Also here, because all three are about the same claim — that this library works where it says it does: examples/openkal builds these sources above openkal, the portable kernel ABI, and makes a real HTTPS request through kal_net_connect. It is a separate CI job because whether MSG_NOSIGNAL or SO_NOSIGPIPE exists is decided by the C library rather than the operating system, and a #ifdef that is wrong about that compiles cleanly on the gcc job and fails there — which is how 5e7d66f reached master. templates/ ships three starting points for `mcpp new --template tinyhttps`, one per entry point. tools/template_smoke.sh renders and builds them against the working tree, because `mcpp new` can only reach a template that is already published, and checking them after the release makes the first user the one who finds out. It has already caught one: import std carries no stdout macro, so a progress bar flushed through it compiled here and not in a generated project. mcpp.lock is removed and ignored. The file's own header says it does not pin anything — "index dependencies are re-resolved from their constraints each time" — so for a library, whose consumers resolve from its constraints, it recorded one machine's resolution and changed nothing about anyone else's. Eight of the ten mcpplibs packages already ignore it, the scaffold template among them.
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
2 participants
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.
Supersedes #12, which it carries as its first commit with @Cloud_Yun's authorship intact. Fixes #11.
It is one PR rather than two because #12's head is on a fork I cannot push to, and the second half is what makes the first half usable on the library's own defaults.
The reported defect, confirmed
One program, one source file, built against
masterand against this branch withtarget/wiped between them:events=0confirms the mechanism and not just the symptom: httpbin's teapot document does contain a blank line, soSseParserfinds a boundary — the block it extracts has nodata:,event:orid:field, sodispatch_event_returns without pushing. The bytes reached the parser and died there.The fix is placed correctly.
dispatchis the single funnel for every body byte on both framing paths, andcaptureBodyis decided after the headers, wherestatusCodeis final (send_streamfollows no redirects). Capturing there is also better than reaching intoSseParser's buffer, which is not the whole body once any event has been dispatched.1. A declared
Content-Length— which is why the test had to close the connectionsend_streamhad no branch for it: a response that was not chunked was read until the connection closed, whatever its headers said. On the library's defaults (keepAlive = true) the server does not close, and the loop ran untilreadTimeoutMsexpired. The evidence was in the test itself:Measured against
/status/418:The body arrived either way; on the configuration a caller actually gets, it arrived a full read timeout late — a minute, at the shipped default of 60000.
send()has had this branch throughout, which is the same asymmetry between the two entry points that #11 is about.The live test now runs on the default configuration and asserts the elapsed time. A test that closes the connection to make the loop end is examining the one arrangement in which the defect does not appear.
2. A chunk size that does not parse is not a terminal chunk
parse_hexreturns what it accumulated when it meets an unrecognised character, and0for an empty line — andread_linereturns an empty line on a timeout or a closed connection. A stream that was cut short therefore read as a stream that ended cleanly, and the loop reported success.#9 established
parse_chunk_size_linefor exactly this and it reacheddownload_to_filealone;sendandsend_streamwere left on the old parser.3.
Content-Lengthwas parsed by keeping the digitsMeasured, by compiling that parser on its own:
The last two are the ones no care at the call site could recover from, because what it receives is a plausible number.
parse_content_lengthis exported and shaped likeparse_chunk_size_line, for the reason #9 gave — it is the half of the body framing that can be examined without a server — and both readers use it.Criteria
Six unit tests over the two pure parsers, and two live ones: the failed stream on the default configuration with the elapsed time asserted, and a chunked 2xx asserted to leave
bodyempty and return promptly, since the chunked branch is the one this change restructured around.17 tests from 6 test suitespass, plus 3 intest_resolver.And it works on openkal
tinyhttps builds and runs on
openkal-musl: statically linked, zeroPT_INTERP, TLS and DNS working, and this change behaves identically there.One thing invisible from inside this library, recorded so nobody has to re-derive it.
Socket::wait_readableispoll_fd, and on openkal-linux before 0.7.1 the bounded wait was performed on the descriptor below the one it then transferred on — soreadTimeoutMssilently did nothing:Nothing here needs to change for it; it is fixed beneath, in mcpplibs/openkal-linux#19.