Add asynchronous chunked content provider for streaming responses by ssubbotin · Pull Request #1213 · CrowCpp/Crow

ssubbotin · 2026-08-05T18:42:45Z

Closes #16.

Why this is needed

The original synchronous chunk provider is useful when the next chunk is immediately available. It is a poor fit for long-lived streams whose producer may wait for disk, network, or another worker thread. The provider runs as part of the connection write path, so waiting inside it blocks that Asio worker. With a small server pool, one slow stream can delay unrelated requests and make shutdown or peer-disconnect handling unresponsive.

Moving response::end() to an application-owned thread does not solve that safely. Response and connection state still belong to the connection executor, so driving the write state machine from another thread introduces lifetime and concurrent-access hazards. The asynchronous API keeps all socket and response transitions on the executor while allowing the application to obtain the next chunk elsewhere.

The earlier work in #1151 established the response-level chunk-provider direction. This PR keeps that model and preserves the synchronous provider API, then adds the asynchronous completion contract needed when producing a chunk can take an arbitrary amount of time.

API and execution model

set_async_chunked_content_provider() invokes a provider for one chunk at a time. The provider returns promptly and completes that request inline or from any thread with more, done, or abort. Crow posts every accepted result to the connection executor and uses an asio::async_write chain for headers, data chunks, and the terminating frame.

The existing set_chunked_content_provider() API remains available for sources that can provide each chunk synchronously.

What changed

One provider request and one socket write may be outstanding per response. The next chunk is requested only after the previous write completes, providing socket backpressure.
Header, chunk payload, and terminating-frame buffers remain alive until their asynchronous writes complete.
done sends the terminating frame and permits keep-alive reuse. abort, provider exceptions, publication failures, write failures, peer closure, retained-input overflow, and server shutdown finish uncleanly and close the connection.
Peer closure is detected while Crow is waiting for an asynchronous provider, without concurrent reads during socket writes. Pipelined input is retained and parsed after a clean response.
Active transfers are tracked by their worker and are completed on that worker during shutdown. Duplicate and late completions are discarded safely.
HEAD suppresses provider execution while preserving the correct representation headers. HTTP/1.0 receives a finite 505 response because chunked transfer coding requires HTTP/1.1.
Chunk providers and static-file bodies are mutually exclusive, and enabling chunked output removes a previously set Content-Length.
Completion is reported exactly once with a clean or unclean result, including shutdown and transport failures.

Coverage and documentation

The guide documents both provider APIs and their different execution models. The test suite covers inline and cross-thread completion, responsiveness, backpressure, abort and error paths, publication retry, peer disconnect while waiting, shutdown races, every write phase, HEAD, HTTP/1.0, keep-alive, pipelined requests, framing, and response lifetime behavior.

CI results for the current head are required before merge.

anton-n-petrov

Blocking on the two header bugs; suggestions are optional.

anton-n-petrov · 2026-08-06T12:22:59Z

                if (skip_body)
                {
+                    chunk_provider_ = nullptr;
                    set_header("Content-Length", std::to_string(body.size()));


[bug] For HEAD, end() nulls the chunk provider (good) but then always sets Content-Length to body.size() (usually 0) while leaving Transfer-Encoding: chunked in place. That violates RFC 7230 §3.3.2/§3.3.3 (sender must not send both) and is wrong for HEAD semantics: the length should match what GET would produce, and for streaming length is unknown so TE: chunked without a body and without Content-Length is the right shape. The HEAD test only asserts the literal "body" is absent, so this slips through.

Suggestion: When skip_body and a chunk provider is present, drop the provider, keep Transfer-Encoding: chunked and manual_length_header = true, clear body, and do not set Content-Length. Extend chunked_response_head_request to assert Transfer-Encoding: chunked is present, Content-Length is absent, and the body is empty.

Fixed in 0981e28: when skip_body finds a chunk provider, the providers are dropped, the body is cleared, manual_length_header is set and Content-Length is not written, so a HEAD response keeps Transfer-Encoding: chunked alone. chunked_response_head_request now asserts TE present, Content-Length absent, empty body.

anton-n-petrov · 2026-08-06T12:22:59Z

+        void set_chunked_content_provider(chunk_provider_t provider, std::string content_type = "")
+        {
+            chunk_provider_ = std::move(provider);
+            manual_length_header = true;


[bug] set_chunked_content_provider sets manual_length_header = true and Transfer-Encoding: chunked, but never removes an existing Content-Length header. Any prior set_header("Content-Length", ...) (or other path that added it) is still emitted from write_header_into_buffer, producing both headers on the wire for a chunked response.

Suggestion: Erase Content-Length when enabling the chunked provider (e.g. headers.erase("Content-Length") via the ci_map), and/or document that callers must not set Content-Length with this API. Prefer the erase so the API is hard to misuse.

Fixed in ec62cee: enabling the chunked provider now erases any previously set Content-Length (both overloads go through this path), and the doc comment states it.

anton-n-petrov · 2026-08-06T12:22:59Z

+            {
+                chunk.clear();
+                more = provider(chunk);
+                if (chunk.empty())


[suggestion] Empty chunks are skipped with continue while more remains true. A provider that repeatedly returns true with an empty string (the docs encourage empty chunks when "no bytes yet") busy-loops forever on the connection thread, never yielding, never hitting the deadline (already cancelled), and never completing the response.

Suggestion: Document that empty+true must not spin (caller should block/sleep until data is ready, or return false). Optionally guard with a max consecutive empty iterations, or treat N empty returns as an error and abort the write with a log. At minimum, strengthen the streaming guide so "not ready yet" is not read as a poll-in-a-tight-loop API.

Addressed in 2026027 by documenting the contract (both provider doc comments and the streaming guide): an empty chunk is fine as an occasional occurrence, but a provider with no data at hand should block until data is available or finish the transfer. I deliberately did not add a max-consecutive-empty guard: any threshold would be arbitrary and could break legitimate providers with rare pauses, while the contract makes the intent explicit.

anton-n-petrov · 2026-08-06T12:22:59Z

            headers = std::move(r.headers);
            completed_ = r.completed_;
            file_info = std::move(r.file_info);
+            chunk_provider_ = std::move(r.chunk_provider_);


[suggestion] Move-assignment now moves chunk_provider_ and headers (including Transfer-Encoding: chunked) but still does not move manual_length_header. A moved chunked response can therefore keep TE: chunked while manual_length_header stays false on the target, causing write_header_into_buffer to also inject Content-Length: 0.

Suggestion: Move (or copy) manual_length_header (and ideally skip_body / compressed) in operator=, matching the other response state that affects wire format.

Fixed in f5ab73f: move assignment now carries manual_length_header, skip_body and (under CROW_ENABLE_COMPRESSION) compressed.

anton-n-petrov · 2026-08-06T12:22:59Z

+            while (more && !ec)
+            {
+                chunk.clear();
+                more = provider(chunk);


[suggestion] provider(chunk) is invoked outside any try/catch. Route-level exception handlers only wrap handler execution, not the later write path. A throwing provider aborts mid-stream (headers may already be sent, no final chunk), can escape into the Asio completion stack, and leaves the connection unclean.

Suggestion: Catch exceptions around the provider call, log, stop the chunk loop, close the connection, and avoid restarting keep-alive reads. Document that providers should not throw.

Fixed in aefa8f4: the provider call is wrapped in try/catch; an exception is logged and treated as an abort (no terminating frame, forced close, completion handler gets clean == false, no keep-alive restart). Covered by the new chunked_response_throwing_provider test, and the doc comment now says providers should not throw.

anton-n-petrov · 2026-08-06T12:22:59Z

+    app.wait_for_server_start();
+
+    HttpClient client(LOCALHOST_ADDRESS, 45451);
+    client.send("GET /chunks HTTP/1.0\r\n\r\n");


[suggestion] All new chunked tests speak HTTP/1.0. Chunked transfer coding is an HTTP/1.1 feature; real 1.0 clients are not required to understand it. The tests pass only because the harness is a raw TCP client looking for the 0\r\n\r\n trailer.

Suggestion: Use HTTP/1.1 with a Host header in these tests (and/or auto-upgrade the response version when chunked is used). Optionally assert behavior for keep-alive + a second request on the same connection after a chunked response.

Fixed in 8bf9162: all chunked tests now speak HTTP/1.1 with a Host header, and chunked_response additionally issues a second request on the same kept-alive connection to verify the connection is returned to reading after a chunked transfer.

anton-n-petrov · 2026-08-06T12:22:59Z

+    for (std::string::size_type pos = response.find("400\r\n"); pos != std::string::npos;
+         pos = response.find("400\r\n", pos + 1))
+        ++seen;
+    CHECK(seen >= chunk_count);


[nit] CHECK(seen >= chunk_count) is a weak lower bound. A regression that duplicated chunk headers would still pass. Body integrity is never checked for the large case.

Suggestion: Assert seen == chunk_count and/or verify decoded payload length chunk_count * chunk_size after a minimal chunk decode.

Fixed in 485e609: the test now decodes the chunked body frame by frame and asserts seen == chunk_count plus the total decoded length of chunk_count * chunk_size.

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.

anton-n-petrov

Requesting changes for incomplete framing on write failure in do_write_chunked: unlike chunk_result::abort, a mid-transfer ec can leave keep-alive and restart do_read() without a terminating chunk. Please force-close (and skip the keep-alive restart) whenever the chunked body did not finish cleanly.

Other notes (completion-handler exceptions, HEAD not calling the completion handler, static vs chunked mutual exclusion) are non-blocking suggestions. Prior review items look fixed.

anton-n-petrov · 2026-08-06T15:26:29Z

+
+            // The deadline was cancelled for the duration of the transfer, so a kept-alive
+            // connection has to be put back into reading state explicitly.
+            if (!aborted && !close_connection_ && need_to_start_read_after_complete_)


[bug] On a mid-transfer write failure (ec set after a chunk or after headers), the code does not force-close the socket the way chunk_result::abort does. The terminating frame is correctly omitted (result == done && !ec), but if the connection is keep-alive (!close_connection_ and need_to_start_read_after_complete_), do_read() is restarted on a connection whose HTTP message framing is incomplete. Abort was carefully designed so clients see a truncated body; write errors produce the same incomplete framing without the same connection lifecycle. (Socket write errors often imply a dead peer, but when the socket remains open this desynchronizes keep-alive.)

Suggestion: Treat write failures like abort for connection policy: if ec after any incomplete chunked body (no successful terminating frame), shut down/close the adaptor and do not restart do_read(). e.g. const bool force_close = aborted || static_cast<bool>(ec); and gate the keep-alive restart on !force_close.

Fixed in 2fb5f6d: a write failure now follows the same connection policy as an explicit abort (force_close = aborted || ec) - the socket is shut down and keep-alive reads are not restarted on a connection with incomplete framing.

anton-n-petrov · 2026-08-06T15:26:29Z

+
+            if (completion_handler)
+            {
+                completion_handler(result == response::chunk_result::done && !ec);


[suggestion] Provider exceptions are caught and converted to abort, but completion_handler(...) is invoked without a try/catch. If the handler throws, control never reaches the subsequent close_connection_ shutdown, res.clear(), or keep-alive do_read() restart, and the exception can escape into the Asio callback stack—the same class of problem previously fixed for providers.

Suggestion: Wrap the completion-handler call in try/catch (log and continue cleanup), matching the provider path. Document that the handler should not throw.

Fixed in 2fb5f6d: the completion handler is now invoked under try/catch (logged and swallowed), so an exception cannot skip the cleanup or escape into the Asio callback stack. Documented that the handler should not throw.

anton-n-petrov · 2026-08-06T15:26:29Z

+                        // "Transfer-Encoding: chunked" is kept and "Content-Length" is not
+                        // set (RFC 7230 forbids sending both at once). The body itself is
+                        // skipped, so the provider is dropped without being called.
+                        chunk_provider_ = nullptr;


[suggestion] On HEAD (skip_body), providers are nulled without being called (good, and covered by tests/docs), but chunk_complete_ is left set and is never invoked: write goes through do_write_general because is_chunked_type() is already false, and res.clear() later discards the handler. The docs present the completion handler as the place “to release the source of the data,” so HEAD silently skips that cleanup path. RAII via provider captures still works when the std::function is destroyed; relying only on the completion handler does not.

Suggestion: Either invoke the completion handler for HEAD (e.g. clean=true if headers were sent, or document a dedicated meaning), or explicitly document that HEAD never runs the completion handler and that resource lifetime must be tied to the provider (or other RAII), not only to set_chunked_completion_handler.

Fixed in 8c6cf80: the completion handler now runs for HEAD as well (with clean == true) at the point the providers are dropped, so it stays the single release point for the data source. Covered by an extension of chunked_response_head_request and documented in the guide.

anton-n-petrov · 2026-08-06T15:26:29Z

+        /// and "Content-Length" must not be sent together.
+        void set_chunked_content_provider(chunk_provider_ex_t provider, std::string content_type = "")
+        {
+            chunk_provider_ex_ = std::move(provider);


[suggestion] set_chunked_content_provider does not clear file_info, and set_static_file_info_unsafe does not clear chunk providers. complete_request prefers is_static_type() over is_chunked_type(). If both are set, the static path sends the raw file body while headers may still include Transfer-Encoding: chunked (and possibly a Content-Length from the file after a reverse call order)—illegal/conflicting framing relative to RFC 7230.

Suggestion: Make body sources mutually exclusive: in set_chunked_content_provider, clear file_info (and ideally body); in set_static_file_info_unsafe, null chunk providers/completion and erase Transfer-Encoding when installing Content-Length. Optionally assert/log if both were set.

Fixed in 408cadf: the setters now discard each other's body source. set_chunked_content_provider clears file_info and the string body; set_static_file_info_unsafe drops the providers, the completion handler and the Transfer-Encoding header before installing Content-Length. Whichever source is configured last wins; covered by chunked_provider_excludes_other_body_sources.

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 · 2026-08-06T17:51:25Z

All four comments addressed (2fb5f6d, 8c6cf80, 408cadf). The red CI had two causes: most jobs died on a GitHub infra hiccup ("Service Unavailable" while resolving actions), and the one real failure (http_method on macOS) was a regression from the previous round - moving skip_body in the response move-assignment let a handler-built response overwrite the flag the router sets for HEAD. Fixed in 44a443e by keeping skip_body out of the move. Full suite passes locally (133 cases / 1144 assertions).

anton-n-petrov

LGTM — re-reviewed after the follow-up commits (HEAD headers, Content-Length erase, abort/write-error handling, completion on HEAD, static vs chunked exclusivity, tests).

Prior review items are addressed. Residual note only: empty chunk + more can still spin the connection thread; that's documented as contract, fine for merge.

gittiver · 2026-08-09T09:31:37Z

Just FYI - there was former approach: https://github.com/CrowCpp/Crow/pull/1151/changes

Hadn't the time time yet, to look into your proposal. But thanks for your work.

BR,
Frank

gittiver · 2026-08-12T08:58:57Z

+            // provider finished cleanly and every previous write succeeded.
+            if (result == response::chunk_result::done && !ec)
+            {
+                static const std::string last_chunk = "0\r\n\r\n";


constexpr char[] here?

Fixed in b37e15a. The terminator now uses static constexpr char last_chunk[], and the Asio buffer length is sizeof(last_chunk) - 1 to exclude the trailing null byte.

This reverts commit 4661f9e.

anton-n-petrov · 2026-08-17T08:44:43Z

+            if (!ec)
+            {
+                const std::size_t retained_input_limit = max_header_size;
+                if (buffered_input_.size() > retained_input_limit || bytes_transferred > retained_input_limit - buffered_input_.size())


[suggestion] handle_async_chunk_read treats retained-input overflow as an immediate unclean abort before consulting pending_result_ready. A provider can already have published done (or a final nonempty chunk) while the watch read completes with extra pipelined bytes that push buffered_input_ over max_header_size. Those bytes belong to the next request; aborting here omits the terminating frame and reports clean == false for a transfer that otherwise finished. The overflow test (async_chunked_response_closes_when_waiting_input_exceeds_parser_limit) only covers a never-completing provider, so this race is untested.

Suggestion: If pending_result_ready is set, apply that result first and do not append the overflowing tail. Overflow should fail the transfer only while still waiting for the provider (or after applying more, on the next do_read watch). A pending done should still write the last chunk and terminator so the current response completes cleanly.

Fixed in 276037b. A pending provider result now takes precedence over retained-input overflow. The overflowing tail is not appended; done can write its final chunk and terminator cleanly, while more carries the overflow state to the next provider-request boundary and then closes uncleanly. I also added a deterministic regression that queues done before releasing the read completion which crosses the limit.

anton-n-petrov · 2026-08-17T08:44:43Z

+            response::chunk_complete_t completion_handler;
+            std::string chunk;
+            std::string chunk_header;
+            std::string terminator{"0\r\n\r\n"};


[nit] The async terminator is still a per-transfer std::string terminator{"0\r\n\r\n"}. The sync path was switched to static constexpr char last_chunk[] = "0\r\n\r\n" in b37e15a. A static constexpr is actually a better fit for async writes: the bytes have static lifetime, so they outlive asio::async_write without occupying AsyncChunkTransfer.

Suggestion: Use a static constexpr (or the same last_chunk as the sync path) when filling state->buffers in write_async_chunk_terminator.

Fixed in 276037b. The per-transfer string is removed, and write_async_chunk_terminator now uses a function-local static constexpr char[] with static lifetime.

anton-n-petrov · 2026-08-17T08:44:43Z

-                    manual_length_header = true;
+                    if (is_chunked_type())
+                    {
+                        // A response to HEAD must carry the same header fields a GET would


[suggestion] The HEAD branch comment restates the surrounding code and embeds RFC plus connection-lifecycle design history (provider dropped, completion retained for the connection, direct responses complete here). That is more architecture narrative than a constraint the next reader needs.

Suggestion: Cut it to the non-obvious why: HEAD keeps Transfer-Encoding: chunked and omits Content-Length; the connection invokes the completion handler after the HTTP-version check.

Fixed in 276037b. The comment now only records the non-obvious HEAD framing rule and that the connection invokes completion after its HTTP-version check.

ssubbotin · 2026-08-17T12:48:27Z

CI note: the previous macOS, sanitizer, fuzzing, and C++ CodeQL failures all came from the same test-helper compile error under ASIO_NO_DEPRECATED: steady_timer::cancel(error_code&) was unavailable. Commit 7d06d96 switches those calls to the portable zero-argument overload. The workflows for the new head are currently waiting for approval to run.

anton-n-petrov

Summary

This PR adds set_async_chunked_content_provider() on top of the existing synchronous chunked API, with a careful one-in-flight request/write machine, peer-close watch, retained pipelined input, HTTP/1.0 rejection, HEAD skip, and worker-shutdown tracking. The core executor posting, completion latch, and more backpressure look sound and are well tested. Two correctness holes remain: in-flight header buffers alias res.headers while handle() still mutates that map after end(), and a done result that races with retained-input overflow still resumes keep-alive after dropping bytes.

Issue counts by severity

bugs: 2
suggestions: 3
nits: 0

anton-n-petrov · 2026-08-17T19:31:24Z

+            {
+                lifecycle_registry_->track(self);
+            }
+            asio::async_write(


[bug] do_write_async_chunked() starts asio::async_write on buffers_, whose asio::const_buffers point at the std::string keys/values stored in res.headers (write_header_into_buffer does kv.first.data() / kv.second.data()). That write is still in flight when complete_request() returns to Connection::handle(), which then does res.set_header("connection", "Keep-Alive") on every HTTP/1.1 keep-alive request (http_connection.h:243). set_header is headers.erase(key) plus emplace on a std::unordered_multimap. Erase destroys an existing Connection value that is already in buffers_; emplace may rehash and relocate SSO strings such as "chunked" and "text/plain". Previously every complete_request() path wrote headers synchronously, so this mutation happened after the bytes were already on the wire. The new async header write makes it a use-after-free on the common keep-alive path.

Suggestion: Copy header bytes into connection-owned storage before async_write, or skip set_header when the response is already completed (write_header_into_buffer already emits Connection: Keep-Alive from add_keep_alive_). The second option is enough if nothing else mutates res.headers until the header write completes.

Fixed in adb968a. handle() no longer mutates response headers after end() has completed the response, so the strings referenced by the in-flight asynchronous header buffers remain stable. The existing header writer already emits the default keep-alive field. A paused-header-write regression preserves a route-supplied Connection value through completion.

anton-n-petrov · 2026-08-17T19:31:24Z

+                return;
+            }
+
+            const bool resume_input = clean && !force_close && !close_connection_ && need_to_start_read_after_complete_;


[bug] Commit 276037b correctly defers overflow abort when a provider result is already pending, so a terminal done can still be written. The overflow flag is only consulted in request_async_chunk() (http_connection.h:670), which done never reaches. write_async_chunk_terminator() then calls finish_async_chunked(state, true, false), and resume_input is computed without looking at retained_input_overflow. The overflowing read is discarded (http_connection.h:1221) while earlier retained bytes are kept. On a keep-alive connection Crow then process_buffered_input() / do_read() and continues the HTTP stream with a hole in it. A complete second request that arrived under the limit is served, then the next read is the remainder after the dropped overflow — HTTP desynchronization. The new test (async_chunked_response_finishes_cleanly_when_done_precedes_retained_input_overflow) only checks that the current body is terminated; it sends 'x' padding, so the leftover parse fails and hides reuse. Docs still say overflow “aborts the transfer and closes the connection”.

Suggestion: After applying a pending done, if retained_input_overflow is set, write the terminator then close (force_close or close_connection_ = true) and do not resume input. Keep clean == true if you want the current body to count as finished. Extend the test to a valid pipelined second request plus overflow and assert the connection is not reused.

Fixed in adb968a. A pending terminal result still writes its final chunk and terminator with clean == true, but retained-input overflow now forces the connection closed and prevents buffered-input processing. The regression now sends a valid pipelined request before the overflowing tail and verifies that the connection closes without dispatching that request. The streaming guide documents the terminal-result case.

anton-n-petrov · 2026-08-17T19:31:24Z

+            state->completion_handler = std::move(res.chunk_complete_);
+            res.chunk_complete_       = nullptr;
+            async_chunk_transfer_     = state;
+            parser_.stop_after_message();


[suggestion] parser_.stop_after_message() is armed only inside do_write_async_chunked(), which runs when res.end() is called. Crow otherwise allows deferred end(). If the handler installs an async provider and returns without ending, and the current read already contains a pipelined second request, on_message_complete returns 0, the parser continues, and the second handle() overwrites the same res (including the provider) before the first transfer starts.

Suggestion: After the user handler returns, if res.is_chunked_type(), call stop_after_message() immediately so leftover bytes stay unparsed until the first response actually starts (or is abandoned).

Fixed in adb968a. After the route handler returns, Crow now arms stop_after_message() whenever the response has a chunked provider, including deferred end(). The new regression sends two requests in one read, verifies that the second route remains undispatched while the first response is deferred, then verifies normal processing after the first response completes.

anton-n-petrov · 2026-08-17T19:31:24Z

+
+            untrack_async_chunk_transfer();
+
+            // No executor turn remains during worker teardown. Invalidate the request


[suggestion] The comment claims “No executor turn remains during worker teardown,” but destroy_async_chunk_transfer() is the Connection destructor fallback, including Connections that never went through Server worker shutdown. It also restates the invalidate/close sequence and embeds the publication-vs-destruction design.

Suggestion: Keep a one-line why: invalidate the request gate before destroying the connection so a racing complete() cannot post onto a dead executor.

Fixed in adb968a. The comment now records only the ordering constraint: invalidate before destruction so racing completion cannot post to a dead executor.

anton-n-petrov · 2026-08-17T19:31:24Z

+
+            void notify_completion(bool clean) noexcept {
+                response::chunk_complete_t handler;
+                {


[suggestion] The comment on the completion latch narrates which threads run normal completion vs worker shutdown vs destructor fallback. That is architecture history, not a constraint the next reader needs in order to change the lock.

Suggestion: Replace with “once-only: completion may run from the worker or from destruction.”

Fixed in adb968a. The latch comment is reduced to the once-only requirement across worker completion and destruction.

anton-n-petrov

Summary

This PR adds a well-structured asynchronous chunked body provider on top of the existing synchronous API, with careful once-only completion, backpressure (one pending provider call and one write), peer-close watches, retained pipelined input, HTTP/1.0 rejection, and worker-shutdown cleanup. The async write path and its tests look largely correct. The main problems are a test that does not compile, retained pipelined bytes that only the async finish path replays, and a worker-local connection registry that res.end() can mutate from any thread.

Issue counts by severity

bugs: 3
suggestions: 0
nits: 0

anton-n-petrov · 2026-08-18T07:26:28Z

+
+TEST_CASE("async_chunk_transfer_registry_ignores_ordinary_connections_and_unregisters_completed_transfers")
+{
+    crow::detail::connection_lifecycle_registry<LifecycleRegistryProbe> registry;


[bug] connection_lifecycle_registry is a non-template class whose track() method is already templated on the connection type. This test instantiates crow::detail::connection_lifecycle_registry<LifecycleRegistryProbe>, which does not compile and will fail the unittest target.

Suggestion: Drop the template argument and construct crow::detail::connection_lifecycle_registry registry;. track(active_transfer) already accepts shared_ptr<LifecycleRegistryProbe>.

Fixed in bc1789c. The test now constructs the type-erased connection_lifecycle_registry directly. Registry coverage was also split to verify untracking, exactly-once shutdown, and rejection of registration after shutdown.

anton-n-petrov · 2026-08-18T07:26:28Z

+            {
+                need_to_start_read_after_complete_ = false;
+                start_deadline();
+                do_read();


[bug] handle() calls parser_.stop_after_message() for every chunked response, and process_incoming_input() stores unparsed tail bytes in buffered_input_. Only finish_async_chunked() replays that buffer. After a deferred res.end() of a synchronous chunked response (or a HEAD chunked response, which uses do_write_general()), keep-alive completion calls do_read() and leaves buffered_input_ untouched. A second request that arrived in the same TCP read is never parsed; the connection waits for more socket data and the client, waiting for that response, can hang. A later do_read() then assign()s any new bytes over the saved request and corrupts the next parse.

Suggestion: Share the async resume helper: after a clean keep-alive finish, if buffered_input_ is non-empty call process_buffered_input(), otherwise do_read(). Use that from do_write_sync_chunked() and the small-body do_write_general() path, not only from finish_async_chunked().

Fixed in bc1789c. Retained-input replay is now centralized in resume_input_after_response(), which processes buffered input before starting another socket read. The asynchronous, synchronous chunked, and small general-response paths all use it. Regressions cover deferred synchronous chunked and deferred HEAD responses with a pipelined second request.

anton-n-petrov · 2026-08-18T07:26:28Z

+        template<typename ConnectionType>
+        void track(const std::shared_ptr<ConnectionType>& connection)
+        {
+            std::weak_ptr<ConnectionType> weak_connection = connection;


[bug] connection_lifecycle_registry is an unsynchronized unordered_map, with a comment that every method runs on the connection worker. track() is invoked from do_write_async_chunked() via complete_request(), and Crow's documented async pattern (and this PR's own deferred_chunked_response_stops_parsing_at_its_request_boundary test) calls res.end() off the worker. Two connections on the same worker that end() from foreign threads, or a foreign track() concurrent with worker untrack() / shutdown_all(), is a data race on the map.

Suggestion: Either post track() onto the connection io_context so the comment is true, or guard track / untrack / shutdown_all with a mutex. Do not leave the map as worker-thread-only shared state if end() remains callable from any thread.

Fixed in bc1789c. Registry track, untrack, and shutdown are serialized by a mutex. shutdown_all marks the registry closed and moves callbacks out under the lock, then invokes them after releasing it. track returns false after shutdown begins, and do_write_async_chunked aborts before starting its header write when registration is rejected. A concurrent untrack/shutdown regression covers the synchronization boundary.

anton-n-petrov · 2026-08-18T12:37:16Z

@@ -331,6 +418,687 @@ namespace crow
            parser_.clear();


[bug] do_write_static() never calls resume_input_after_response(). handle() sets parser_.stop_after_message() whenever the response is still chunked after the route handler returns, and leftover bytes from that parse land in buffered_input_ with need_to_start_read_after_complete_ == true. The public API then allows replacing that chunked source with a static file (set_static_file_info / set_static_file_info_unsafe explicitly release the chunk provider). end() takes this write path, clears the parser, and returns. The retained pipelined request is never fed, and no further do_read() is started on a keep-alive connection. Tests only cover leftover replay for deferred chunked, deferred HEAD, and async transfers.

Suggestion: After a successful static write on a keep-alive connection, call resume_input_after_response() when need_to_start_read_after_complete_ is set (and discard leftover / skip resume on close or write failure). Add a test that pipelines a second request, configures a chunk provider without ending, then switches to set_static_file_info before end().

Fixed in ed8549b. The static path now records header and file-body write failures, force-closes after an incomplete response, and calls resume_input_after_response() only after a successful reusable response. A regression configures a deferred chunk provider, replaces it with set_static_file_info() before end(), pipelines a second request, and verifies that the provider is released and the retained request is dispatched after the file response.

anton-n-petrov

Summary

This change adds a well-structured async chunked provider (one outstanding request, one write, executor-posted completions, retained pipelined input, HEAD/HTTP/1.0/static-file exclusion) with unusually thorough tests for shutdown, peer close, publication failure, and backpressure. The core lifetime model (weak request + invalidate-under-mutex + worker registry) looks correct. The remaining defects are in the shared keep-alive resume paths that the latest “Resume retained input after static responses” commit only partially updated: a deferred chunked response that is later replaced by a large string body never restarts input, and a failed small general write still replays leftover bytes.

Issue counts by severity

bugs: 2
suggestions: 1
nits: 0

anton-n-petrov · 2026-08-18T22:44:18Z

+            }
+        }
+
+        void resume_input_after_response()


[bug] resume_input_after_response() is the only place that both restarts the connection deadline and replays buffered_input_ after stop_after_message(). The new static, sync-chunked, and async-chunked finish paths call it, and the small-body branch of do_write_general() does too. The streaming branch (res.body.length() >= res_stream_threshold_, default 1 MiB) still only clears the parser and returns. That is now a live gap in the retained-input contract: a handler can install a chunked provider (so handle() sets stop_after_message() and leftover bytes are saved), then later clear() and end() a large string body. need_to_start_read_after_complete_ stays true, no read is armed, and the Connection has no outstanding I/O, so the socket is destroyed instead of serving the pipelined request or the next keep-alive request. Docs claim that clear() plus an ordinary string body restores Content-Length “including on a keep-alive connection.”

Suggestion: After a successful large-body write, call the same resume helper used by the small-body and static paths (if (!close_connection_ && !ec && need_to_start_read_after_complete_) resume_input_after_response(); else clear leftover). Mirror the write_failed handling already added to do_write_static().

Fixed in 99376dc, with regression coverage introduced in d6bc826. The streamed regular-response path now records header and body write failures, skips the body after a failed header write, closes and discards retained input on failure, and calls resume_input_after_response() after a successful reusable response. The regression replaces a deferred async provider with a 20,000-byte body, verifies the full multi-block payload, and verifies that the retained pipelined request is dispatched.

anton-n-petrov · 2026-08-18T22:44:18Z

+                    adaptor_.close();
+                    CROW_LOG_DEBUG << this << " from write (res)";
+                }
+                else if (need_to_start_read_after_complete_)


[bug] After a small general write, keep-alive now goes through resume_input_after_response() whenever !close_connection_ && need_to_start_read_after_complete_, including when do_write_sync() just failed (ec is set). The static path added in this PR correctly requires !write_failed before replaying leftover or starting another read. Replaying buffered_input_ after a failed write can parse a pipelined request and attempt another response on a half-closed socket.

Suggestion: Track the write error the same way do_write_static() does and only resume when the write succeeded: if (close_connection_ || ec) { /* close / drop leftover */ } else if (need_to_start_read_after_complete_) resume_input_after_response();

Fixed in 99376dc, with regression coverage introduced in d6bc826. Small and streamed regular responses now share the same final connection decision: any write error closes the transport and discards retained input; only a successful reusable response may call resume_input_after_response(). The write-failure regression injects a synchronous regular-response failure and verifies that the retained pipelined route is never dispatched.

anton-n-petrov · 2026-08-18T22:44:18Z

+                    state->pending_result_ready = false;
+                    state->pending_result_chunk.clear();
+                    state->read_watch_cancel_requested = false;
+                    finish_async_chunked(state, false, true);


[suggestion] If raw_socket().cancel() fails while a provider result is already accepted, the accepted done/more payload is dropped and the transfer is aborted. That is the common path (a read watch is almost always active once request_async_chunk() returns), so a platform or adaptor where cancel() returns operation_not_supported will fail every async stream after the first pending watch, even though the provider already received true.

Suggestion: On cancel failure, either apply the pending result immediately (and ignore a later watch completion unless it is a real peer error) or treat cancel failure as “wait for the in-flight read,” rather than discarding a result Crow already published.

I evaluated both proposed fallbacks and kept the fail-closed behavior. Applying the pending result while the closure read remains active leaves two owners of the connection read lifecycle: a terminal result can restart keep-alive reading while the old watch still owns read_in_progress_, and its later handler can discard retained bytes or leave the connection without a read. Waiting for the watch to complete can stall an accepted provider result indefinitely when the peer sends no input. Crow's built-in TCP, Unix-domain, and SSL adaptors expose Asio socket layers whose cancel(error_code&) supports this watch cancellation; an error there means the transfer cannot safely preserve its state-machine invariant, so aborting and reporting unclean completion is the safe library behavior. Supporting a custom adaptor whose socket cannot cancel requires a separately designed closure-watch abstraction or per-operation cancellation contract, which is outside this PR's asynchronous chunk-provider scope.

ssubbotin · 2026-08-19T10:01:13Z

Follow-up self-review completed for the current head (55e0b578d).

In addition to the two reported retained-input fixes, the review found and corrected two issues in the same path:

Large regular bodies previously used do_write_sync() for each 16 KiB block. That helper clears response storage, while the loop retained a pointer into the body. The streamed branch now writes blocks directly and clears the response once after the loop.
The new deferred-response regressions now publish their test callback on the next connection-executor turn, ensuring the parser has stored the pipelined tail before the response is completed.

The audit covered body-source transitions, framing, retained-input replay or discard, write failures, asynchronous transfer phases, exactly-once completion, shutdown ownership, built-in TCP/Unix/SSL adaptor cancellation, HTTP/1.0 and HEAD behavior, public documentation, portability branches, commit metadata, and accidental internal references. The branch is mergeable with current master and the static formatting/diff checks are clean.

All workflows for the current head are waiting for maintainer approval (action_required), so executable CI evidence is still pending. Please approve the current runs when convenient.

anton-n-petrov

Summary

This PR adds a carefully structured asynchronous chunked body provider on top of a new synchronous set_chunked_content_provider() API, with one in-flight provider request, one write, executor-posted completions, retained pipelined input, HEAD/HTTP/1.0 handling, and worker-shutdown tracking. Prior review defects (HEAD framing, Content-Length/TE coexistence, write-failure keep-alive, leftover replay across write paths, registry races, header-buffer UAF after end()) appear addressed in the current head, and the tests cover the state machine unusually thoroughly. Remaining issues are documentation/thread-affinity inaccuracies around Crow's deferred end() model and a small exception-safety hole on the provider-thread completion callback; I did not find a remaining correctness hole in the keep-alive, overflow, or shutdown paths.

Issue counts by severity

bugs: 0
suggestions: 2
nits: 1

anton-n-petrov · 2026-08-19T13:00:24Z

+        ///
+        /// The body is sent using `Transfer-Encoding: chunked`, so its size need not be known
+        /// in advance, which makes it suitable for bodies of arbitrary or unknown length. The
+        /// provider runs on the connection thread while the response is being written. The


[suggestion] The API comment (and the streaming guide) says the synchronous provider “runs on the connection thread while the response is being written.” do_write_sync_chunked() actually runs on whatever thread called response::end() / complete_request(). Crow’s documented deferred-end() pattern — including this PR’s own deferred_synchronous_chunked_response_replays_pipelined_input test — invokes end() off the worker. A blocking sync provider then blocks that foreign thread, not the connection executor, and resume_input_after_response() (deadline restart plus leftover parse) also runs there. The guide repeats the same claim for leftover replay (“parses the saved bytes on the connection executor”) and for the completion handler (“runs on the connection’s thread”).

Suggestion: Document the actual thread: the thread that calls response::end(). Either post synchronous chunked writes (and leftover replay) onto the connection io_context if executor affinity is the intended contract, or update http_response.h, docs/guides/streaming.md (provider note, completion-handler paragraph, leftover-replay paragraph) to say they run on the end() thread.

Fixed in b2ef24d, with executor-affinity coverage in 539c5c7. Deferred response finalization now uses asio::dispatch on the connection io_context: an end() call already running on that executor keeps the existing immediate behavior, while an end() call from another thread queues complete_request() on the connection executor. Synchronous providers, socket writes, retained-input replay, deadline changes, and normal completion handlers therefore remain on the connection thread. The guide now states this explicitly, and the regression compares the deferred synchronous provider and completion-handler thread with the route's worker thread.

anton-n-petrov · 2026-08-19T13:00:24Z

+                if (request->completed)
+                {
+                    lock.unlock();
+                    CROW_LOG_WARNING << "An asynchronous chunk completion callback was invoked more than once.";


[suggestion] The async complete callback is specified not to let exceptions escape on the provider thread, and the publication-failure path logs under a nested try/catch for that reason. The duplicate-completion branch unlocks and then does CROW_LOG_WARNING << ... with no protection. CROW_LOG allocates and writes to std::cerr; bad_alloc or a throwing log handler will propagate into the application’s provider/callback thread, which is the same class of problem already hardened for asio::post failures.

Suggestion: Log the duplicate invocation with the same log_async_chunk_publication_failure (or an equivalent try/catch) used on the publication-failure path, then return false.

Fixed in b2ef24d. Duplicate completion logging now goes through a dedicated noexcept helper that contains every logging exception, so the callback still returns false without allowing logger allocation or handler failures to escape through the provider thread.

anton-n-petrov · 2026-08-19T13:00:24Z

+            // completion handler have to be taken out of it before the loop starts.
+            auto provider = std::move(res.chunk_provider_ex_);
+            res.chunk_provider_ex_ = nullptr;
+            if (!provider && res.chunk_provider_)


[nit] do_write_sync_chunked() still has a fallback that wraps res.chunk_provider_ if chunk_provider_ex_ is empty. The public chunk_provider_t overload always installs a wrapper into chunk_provider_ex_ and nulls chunk_provider_, so this branch is unreachable through the API.

Suggestion: Drop the chunk_provider_ fallback (or, if an internal caller still assigns it, add a comment naming that caller). Treat an empty chunk_provider_ex_t the same way as an empty async provider if that is the intended missing-callable policy.

Fixed in b2ef24d. Repository-wide assignment search confirmed that the public bool-provider overload always wraps its callable into chunk_provider_ex_ and no internal caller assigns chunk_provider_. The fallback and the redundant private member have been removed. An empty extended provider follows the existing clean empty-provider behavior.

ssubbotin · 2026-08-19T13:47:58Z

The previous CMake run exposed branch-related test and portability issues which are addressed in 539c5c72c and b2ef24df9:

deferred response finalization now has connection-executor affinity;
affected server tests wait for the listener before connecting;
moved provider and completion callables are explicitly cleared in the source response, avoiding libc++'s valid non-empty moved-from std::function state;
the new affinity regression verifies the synchronous provider and completion handler run on the route's worker thread.

The prior run's Bazel matrix, fuzzing, and both CodeQL analyses passed. The current-head workflows are waiting for maintainer approval.

ssubbotin added 4 commits August 5, 2026 20:33

Add chunked content provider to response

5835259

Write chunked responses in the connection

5d6c835

Add tests for chunked responses

5a52b50

Document chunked responses

a96ac1f

anton-n-petrov suggested changes Aug 6, 2026

View reviewed changes

ssubbotin added 8 commits August 6, 2026 14:35

Strengthen the large body test with real chunked decoding

485e609

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.

anton-n-petrov suggested changes Aug 6, 2026

View reviewed changes

ssubbotin added 4 commits August 6, 2026 19:47

anton-n-petrov approved these changes Aug 6, 2026

View reviewed changes

gittiver reviewed Aug 12, 2026

View reviewed changes

Add async chunk provider contract test

0a5759a

ssubbotin mentioned this pull request Aug 14, 2026

Validate asynchronous chunk provider development ssubbotin/Crow#1

Draft

ssubbotin added 6 commits August 14, 2026 12:38

Add async chunk provider contract

989da48

Add async chunk connection tests

40d3823

Implement async chunk response writing

0c5bd1d

Defer reads during async chunk transfers

daa23f5

Add async chunk responsiveness tests

e2a1e15

Harden async chunk responsiveness tests

6ddf86a

Preserve pipelined input during async responses

a32e2e9

ssubbotin changed the title ~~Add chunked content provider for streaming responses~~ Add asynchronous chunked content provider for streaming responses Aug 14, 2026

ssubbotin added 9 commits August 14, 2026 17:20

Add async publication retry regression

ba689f0

Allow retry after async publication failure

29ce4dd

Contain async publication recovery

cd062c9

Add waiting peer lifecycle regressions

d2801a8

Detect peer closure during async chunk waits

fed1ffb

Document async chunk failure outcomes

afdf007

Use constexpr storage for the last chunk

4661f9e

Revert "Use constexpr storage for the last chunk"

8d82e11

This reverts commit 4661f9e.

Use constexpr storage for the last chunk

b37e15a

anton-n-petrov reviewed Aug 17, 2026

View reviewed changes

ssubbotin added 2 commits August 17, 2026 14:46

Add retained input completion race coverage

7d06d96

Handle terminal chunks before retained input overflow

276037b

anton-n-petrov suggested changes Aug 17, 2026

View reviewed changes

Preserve async response stream boundaries

adb968a

anton-n-petrov suggested changes Aug 18, 2026

View reviewed changes

Resume retained input across response paths

bc1789c

anton-n-petrov suggested changes Aug 18, 2026

View reviewed changes

Resume retained input after static responses

ed8549b

anton-n-petrov suggested changes Aug 18, 2026

View reviewed changes

ssubbotin added 3 commits August 19, 2026 11:55

Add regular response retained input regressions

d6bc826

Complete regular response lifecycle handling

99376dc

Make retained input regressions deterministic

55e0b57

anton-n-petrov reviewed Aug 19, 2026

View reviewed changes

ssubbotin added 2 commits August 19, 2026 15:44

Add deferred response executor affinity coverage

539c5c7

Keep deferred response finalization on its executor

b2ef24d


		untrack_async_chunk_transfer();

		// No executor turn remains during worker teardown. Invalidate the request

Uh oh!

Uh oh!

Conversation

ssubbotin commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why this is needed

API and execution model

What changed

Coverage and documentation

Uh oh!

anton-n-petrov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anton-n-petrov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ssubbotin commented Aug 6, 2026

Uh oh!

anton-n-petrov left a comment

Choose a reason for hiding this comment

Uh oh!

gittiver commented Aug 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ssubbotin commented Aug 5, 2026 •

edited

Loading

ssubbotin Aug 17, 2026 •

edited

Loading

ssubbotin Aug 17, 2026 •

edited

Loading

ssubbotin Aug 17, 2026 •

edited

Loading