Validate asynchronous chunk provider development - #1
Draft
ssubbotin wants to merge 88 commits into
Draft
Conversation
A chunk provider can now return chunk_result (more/done/abort) instead of bool: abort closes the connection without the terminating frame, so the client sees a truncated body. An optional completion handler reports whether the body was written cleanly. The bool provider overload keeps its exact behaviour by wrapping into the new one.
A HEAD response to a chunked route used to get Content-Length: 0 while Transfer-Encoding: chunked was still set, sending both headers at once (forbidden by RFC 7230) and misrepresenting what a GET would return. Now the provider is dropped, the body stays empty, Transfer-Encoding: chunked is kept and Content-Length is not set.
A handler that set Content-Length before calling set_chunked_content_provider() would send both Content-Length and Transfer-Encoding: chunked, which RFC 7230 forbids. The header is now erased when the provider is installed; the bool overload delegates to the chunk_result one, so both are covered.
skip_body, manual_length_header and (under CROW_ENABLE_COMPRESSION) compressed were left at their defaults when a response was move-assigned. A moved chunked response would then have manual_length_header == false and write_header_into_buffer() would append Content-Length: 0 next to Transfer-Encoding: chunked.
The provider used to be called outside any try/catch, so an exception would propagate into the Asio write path. It is now caught in do_write_chunked(), logged, and handled exactly like chunk_result::abort: no terminating frame, forced close, completion handler called with clean == false, no return to keep-alive reading.
An empty chunk is allowed as an occasional occurrence; a provider that has no data yet should block until data is available or finish the transfer, since returning empty chunks in a tight loop spins the connection thread needlessly. Stated in the doxygen of both provider types and in the streaming guide.
Chunked transfer encoding belongs to HTTP/1.1, so the tests now send HTTP/1.1 requests with a Host header, matching the other 1.1 tests. The basic test also sends a second request on the same connection to verify that a kept-alive connection goes back to reading state after the chunked transfer.
Counting occurrences of the hex size line only proved a lower bound. The test now walks the chunked body frame by frame and checks the exact frame count and the exact decoded body length.
The router marks a HEAD request by setting skip_body on the connection's response before the handler runs. Copying the flag from the source response in operator= let a handler that assigns a freshly built response reset it, so HEAD responses to plain routes carried the GET body again (caught by the http_method test).
…andler A write failure after the headers or a chunk leaves the message framing incomplete: the terminating frame is never sent. Restarting keep-alive reads on such a connection desynchronizes it, so write errors now follow the same connection policy as an explicit abort: force the close and do not reuse the socket. The completion handler is also invoked under try/catch now, matching the provider: an exception escaping it skipped the connection cleanup and could propagate into the Asio callback stack.
The docs present the completion handler as the place to release the source of the data, but a HEAD response dropped the provider and never invoked the handler: the write went through the general path and res.clear() discarded it silently. The handler now runs (with clean == true) at the point the providers are dropped, so it stays the single release point regardless of the request method.
complete_request prefers the static path over the chunked one, so a response carrying both sent the raw file bytes while the headers still announced "Transfer-Encoding: chunked" next to the file's "Content-Length" - conflicting framing either way the calls were ordered. Each setter now discards the other body source together with its framing header: the source configured last wins.
ssubbotin
changed the title
…eview Review follow-up covering the latest review and a fresh full-diff pass. Protocol: - Honor an application-supplied "Connection: close" (RFC 9112 §9.6): any case-insensitive close token in the response Connection field list closes the connection after the response, keep-alive is never advertised on a closing connection, and a server-initiated close is made explicit when the application supplied no Connection header. - Frame 205 Reset Content as bodyless with an explicit "Content-Length: 0", and normalize bodyless statuses before the HTTP/1.0 chunked rejection so HTTP/1.0 clients receive the bodyless response instead of a 505. - Normalize handler-returned interim (1xx) statuses to a final 500. - Capture HEAD representation metadata after middleware and compression. - Skip compression for chunked responses and drop an application "Trailer" header alongside the other chunked framing normalization. Concurrency and lifetime: - Keep the write deadline of an in-flight transfer when the connection is already marked to close. - Run route-local after handlers on the connection executor when end() arrives from a foreign thread, and report completion even if one throws. - Make response::is_alive() callable from foreign threads: the helper reads shared atomic connection state and its swap is serialized with the connection; the guide now names the exact any-thread surface. Failure paths: - shutdown_all() is no longer noexcept (its copies can allocate), worker shutdown releases the registry entry of an already-latched deferred response, transfer initiation survives a throwing allocation by aborting the transfer, and end(body) appends before latching. Cleanups: unused <limits> include, the do_write_chunked() forwarder, and history-narrating comments rewritten as invariants. The guide's pipelining paragraph now matches the code, completion-handler semantics are described for replaced body sources and exceptions, and the HTTP/1.0 505 choice is stated as a rationale. Tests: application Connection: close (token lists, HTTP/1.0 keep-alive, streams), 205 and interim-status framing, close-marked transfers keep the write deadline, header writes run under the deadline, route-local middleware executor confinement and throwing after handlers, HEAD after body-changing middleware, later-packet requests behind a deferred response, foreign-thread is_alive(), and a robust read in the static HEAD test (a response above asio's gather limit legitimately arrives in two segments).
…idance The 1xx-to-500 normalization erased only the framing headers, so a compressed or handler-supplied Content-Encoding (and a Trailer header) survived onto the synthesized plain-text 500. The block now erases both, the compression pass skips interim and bodyless statuses instead of coding a body that is about to be discarded, and suppress_response_body_for_status() erases Trailer for 204/205/304 and Content-Encoding for 205 (204 and 304 keep representation metadata). request_async_chunk() allocates a request object, arms the idle timer, and builds the completion functor; a throw after the chunked headers were written stranded the client with no watch read, no idle timer, and a cancelled write deadline. The body now runs under a try/catch that finishes the transfer unclean, covering all three call sites, and the remaining unguarded write launches (response header, chunk frame, terminator) sit inside try blocks with the same failure path. The stalled-client test closed the stalled socket before asserting that the transfer was still in flight; the abort driven by the peer closure could win that race, and on Windows IOCP it reliably did. The completion state is now sampled before the sockets close, and the test server uses a 30 s write deadline so a slow runner cannot trip the default 5 s one mid-test. The streaming guide and the provider doc-comments told a synchronous provider with no data to block briefly; the provider shares the connection executor with the peer watch, the stream timers, and every other connection on that worker, and empty chunks are re-requested through the executor. The guidance now says to return promptly, to use the asynchronous provider when data may not be ready, and not to poll with empty chunks. Tests: the 1xx route sets a body plus manual Content-Encoding and Trailer and asserts the 500 carries neither; the bodyless-status test covers the Trailer and Content-Encoding matrix for 204/205/304; a compression-gated test verifies that discarded bodies are never compressed while ordinary responses still are.
A response assigned from a freshly built one lost its pending completion handler: operator= moved the source's empty handler over the destination's, so nothing ever reported the abandoned stream, and the connection's last-resort report had nothing left to call. Crow itself takes that path, because the default exception handler assigns response(500) over the route's response, and the value-returning route wrappers assign over a handler that a before-handler installed. The handler now transfers the way set_chunked_completion_handler() installs one: a source that carries a handler replaces the destination's, a source that carries none leaves the destination's in place, so the write that replaces the stream reports through it, the same way a static file configured over a provider already does.
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
1 participant
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.
CI-only draft for test-driven development of the asynchronous chunk provider proposed in CrowCpp#1213.