Add a request body sink, with FileBodySink as a file-backed wrapper

[anton-n-petrov]

Addresses #1064, together with #1234.

PR 2 of 2

This is the other half of the split of #1233, following @ssubbotin's review ("Step 2: the spool, on an internal sink seam") and answering @gittiver's comment that a file is not always available (embedded/limited targets). #1233 is closed in favor of #1234 and this PR.

Branched from feature/max-body-size (#1234) at 225034f8e, since size limits here come from max_body_size, not a file-only knob — this PR's diff carries that PR's first two commits (fa21b7fa1, 225034f8e); the sink-only changes are 225034f8e..HEAD. #1234 has since gained its own review fixes (lingering close on rejection, dropping req.body's reserve(), explicit skip_body on error responses); this PR reimplements each independently rather than re-merging, so the size-limit mechanics are reviewed once in #1234 and this diff stays sink-focused.

What changed vs. #1233

#1233 wrote every body straight to a file, reopened it by path, and drained an over-limit body instead of closing. This PR replaces that with:

• .body_sink(factory): the primitive — a per-request BodySink (write()/finish()) that the parser feeds from on_body, before the Host header check, before middleware, and before 100 Continue. No filesystem required, per @gittiver. A sink that throws, or returns false, ends the request as a 500 with the connection closed; the route handler never runs.
• crow::FileBodySink: the built-in file-backed sink, handed to .body_sink() via FileBodySink::factory(directory = {}) (empty: system temp directory). Descriptor is created O_CREAT|O_EXCL|O_WRONLY (Windows: CREATE_NEW), mode 0600 — no mkostemp, so no glibc dependency — and kept open (not reopened by path) until finish(). The file is unlinked when the sink is destroyed unless FileBodySink::from(req)->keep() was called.
• crow::FileBodySink lives in its own opt-in header (crow/file_body_sink.h), excluded from the amalgamated crow_all.h; the tiny BodySink/BodySinkFactory interface (crow/body_sink.h) stays in the amalgam.
• Calling .body_sink() again on the same route is last-call-wins — it just replaces the stored factory.
• The sink is only opened when a body is actually coming (has_incoming_body()), not for GET/Content-Length: 0.
• req.body is never pre-reserve()d on a sink route — the size-limit reservation in Add a request body size limit that closes over-limit connections #1234 is conditional on !uses_body_sink(), so a route using the sink doesn't pay the full upload size in RAM before the sink even opens (which was the whole point of moving off req.body in the first place).

API

#include "crow.h"
#include "crow/file_body_sink.h" // opt-in: not pulled in by crow.h

crow::SimpleApp app;
app.max_body_size(64ull * 1024 * 1024);

CROW_ROUTE(app, "/upload")
  .methods(crow::HTTPMethod::Put, crow::HTTPMethod::Post)
  .body_sink(crow::FileBodySink::factory("uploads")) // empty: system temp directory
  ([](const crow::request& req) {
      auto* file = crow::FileBodySink::from(req);
      if (!file)
          return crow::response(200); // e.g. an empty PUT
      // req.body is empty; the bytes are in file->path()
      return crow::response(200);
  });

A custom sink just implements crow::BodySink (write()/finish()) directly — no filesystem needed. See docs/guides/body-file.md for the full guide, including the empty-body edge case, keep() lifecycle, and the RTTI/opt-in rationale.

multipart/form-data is unaffected — it's still parsed from req.body; saving individual multipart parts to disk is a separate feature.

Testing

tests/body_file_tests.cpp covers in-memory vs. file vs. custom-sink routes, keep() surviving a copied request, the file staying open until the full response is sent, size-limit interaction (including chunked, over limit on a sink route), disconnect cleanup, concurrent uploads getting distinct paths, .body_sink() last-call-wins, a factory returning nullptr (keeps the body in req.body), and the sink failure matrix (write()/finish() returning false, throwing std::exception, throwing a non-std::exception, a throwing factory, per-request open failure, and a large in-flight upload finishing before the 500 is read). Full suite (ctest/unittest) passes: 1150 assertions, 153 test cases.

[ssubbotin]

Reviewed at 7767c4ca1; the sink-only part is 225034f8e..HEAD. I read the diff and the surrounding parser and connection code in full, built the branch with -DCROW_BUILD_TESTS=ON, and checked the load-bearing claims against a server built from these headers. Items marked [measured] were reproduced against that running server. Everything I say about the size limit itself is in my #1234 comment; this one is about the sink and the file wrapper.

The branch is healthy: the full suite passes, 1104 assertions in 143 test cases, and the tests now use ephemeral ports. The file sink fixes what #1233 was sent back for: the descriptor is created with O_CREAT|O_EXCL|O_WRONLY|O_CLOEXEC, mode 0600, kept until finish(), the retry loop only retries on EEXIST, close() is checked, partial writes and EINTR are handled. A bodyless GET or a Content-Length: 0 request creates no file [measured], an open failure is a 500 with no 100 Continue [measured], a write failure mid-body is a 500 with the handler never running [measured], an over-cap Content-Length is refused before any file exists [measured], take_body_file() on the handler's own request keeps the file [measured], 20 concurrent uploads get 20 distinct paths and the directory is clean afterwards [measured], and two uploads on one keep-alive socket both work [measured].

My overall read is rework, keeping the seam. The primitive is the right shape and it is what @gittiver asked for; the contract around it has holes that surfaced as soon as I put a misbehaving sink behind it, and the packaging pulls the file code into the core in a way that undoes the embedded argument.

The sink contract

A BodySink runs inside http_parser_execute on the connection's io thread, before the Host check, before any middleware, and before 100 Continue. That is the right place for a body destination and it is fine as a documented contract; what follows are the places where the implementation does not yet honour it.

1. Exceptions from write() and finish() are not caught, and the failure modes are bad

Only the factory call is guarded (http_connection.h:142-154); write() and finish() are called bare (parser.h:103, :123). With a sink whose write() throws std::runtime_error, the exception unwinds through the parser into the worker loop's catch (std::exception&) (http_server.h:205-208), which logs "Worker Crash" and re-enters run(). The client gets no response; the socket stays open until the deadline timer fires about 5 seconds later, because the timer task still owns the connection [measured]. With a sink whose write() throws something that is not a std::exception (throw 42;), the worker thread ends silently under std::async, the process stays alive, and the server keeps accepting connections that are never serviced: a follow-up GET times out [measured]. With the default concurrency, that is the only worker. Handlers are protected from this by Router::handle's catch (...); the sink needs the same boundary, catch (...) mapped to the 500 path.

2. The handler cannot get its sink back

The parser owns the only unique_ptr and request exposes no sink state (parser.h:268). A custom sink is therefore side-effect-only, and the test that exercises one has to smuggle its result out through a shared_ptr<std::string> captured by both the factory and the handler, which is one buffer per route rather than per request. The file case works only because the parser special-cases one subclass with dynamic_cast<FileBodySink*> to copy its path into req.body_file_path (parser.h:124-125) and to forward persist_body_file_ back (:256-259). That is the abstraction admitting it is wrong: the core knows about files after all.

3. take_body_file() still has the copy trap

The flag lives on the request object and the parser reads its own copy (parser.h:258). crow::request r = req; r.take_body_file(); returns a path and the file is deleted anyway [measured]. const plus private mutable plus a template friend hides the same state that #1233 had public; it does not change the semantics.

4. dynamic_cast breaks the one RTTI-free configuration Crow supports

Plain -fno-rtti already fails on asio's typeid, so this is not a new requirement for default builds. With -DASIO_NO_TYPEID -fno-rtti, which asio supports for exactly the embedded targets the maintainer mentioned, #1234 compiles and #1235 fails at the two dynamic_casts (parser.h:124, :256) [measured]. body_sink.h is also included unconditionally by parser.h and crow.h, so <fcntl.h>/<unistd.h> or <windows.h> and the file class are compiled into every translation unit whether or not a file is ever used. The generic interface answers "there is not always a file system"; the packaging does not.

5. Small contract gaps

• The factory runs before handle() sets remote_ip_address (http_connection.h:211), so a sink cannot apply a per-peer policy. One assignment before the factory call fixes it.
• A nullptr from a user factory is a 500 (:144-148). For a user factory, nullptr should mean "no sink, keep the body in req.body"; the file sink's open failure stays 500 because that is an internal error.
• The sink diverts every body regardless of Content-Type, so crow::multipart::message (which parses req.body) cannot be used on a sink route. The guide's "multipart is still parsed from req.body" is true only for routes without a sink; say so.
• The 500 for a mid-body sink failure is delivered the same way as Add a request body size limit that closes over-limit connections #1234's 413: written synchronously, then close() with unread bytes, so the client's send() fails with ECONNRESET first [measured]. The lingering close proposed on Add a request body size limit that closes over-limit connections #1234 covers this path too.

What I would merge instead

Keep the seam, move the file out of the core, and give the handler a handle instead of a path plus a flag.

// body_sink.h: no OS headers, no <filesystem>
struct BodySink {
    virtual ~BodySink() = default;     // if finish() was never called, the body did not arrive in full
    virtual bool write(const char* data, std::size_t length) = 0;   // false or throw: 500, connection closed
    virtual bool finish() = 0;                                      // false or throw: 500
};
using BodySinkFactory = std::function<std::unique_ptr<BodySink>(const request&)>;
// runs on the io thread at headers-complete, before Host validation, middleware and 100 Continue;
// req carries method, url, headers, remote_ip_address; nullptr means "keep the body in req.body"

// routing.h, RuleParameterTraits
self_t& body_sink(BodySinkFactory factory);          // stored as the single new BaseRule field

// http_request.h
std::shared_ptr<BodySink> body_sink;                 // set before the handler runs; copies share it

// crow/file_body_sink.h, opt-in like crow/middlewares/*.h, not included by crow.h
class FileBodySink : public BodySink {
public:
    static BodySinkFactory factory(std::string directory = {});   // empty: temp_directory_path()
    static FileBodySink* from(const request& req);                // nullptr if the route used another sink
    const std::string& path() const;
    void keep();                                                   // do not unlink in the destructor
    ~FileBodySink() override;                                      // close if open; unlink unless keep()
};

• request::body_sink as a shared_ptr removes both dynamic_casts from the core, req.body_file_path, has_body_file(), take_body_file(), the mutable flag, the template friend, and the copy trap in one move: every copy of the request shares the handle, and the file lives as long as the last copy, which is the safe direction. FileBodySink::from() needs a checked downcast of its own inside the opt-in header (a type tag or a dynamic_cast there), since a route may use any sink.
• .body_file(), app.body_file_directory() and the per-route directory become .body_sink(crow::FileBodySink::factory("uploads")). That deletes app.make_body_sink(), app.uses_body_sink(), Router::uses_body_sink(), Router::make_body_sink(), two of the three BaseRule fields, and both create_directories calls in builders (see below). If you would rather keep .body_file() as sugar, it can call the same factory, but it then drags the file header into routing.h.
• Wrap write() and finish() in catch (...) mapped to reject_body(500), and set remote_ip_address before the factory.
• On POSIX, mkostemp(dir + "/crow-body-XXXXXX", O_CLOEXEC) gives O_EXCL, 0600 and uniqueness in one libc call and replaces the 128-attempt loop over random_alphanum, which constructs a std::random_device and seeds an mt19937 per attempt (utility.h:789-800). Keep the CREATE_NEW loop for Windows, with CreateFileW and path.wstring() so non-ASCII directories work.
• Lingering close for the 500, shared with Add a request body size limit that closes over-limit connections #1234.

If the maintainer would rather not commit to a public sink contract yet, the fallback is the internal spool from the #1233 review: .body_file() only, the file class in crow::detail, still with the shared handle on request (so the copy trap and the parser-knows-the-file seam go away either way), and the public sink as a follow-up. I would take the seam now, since it is what was asked for and it is the smaller core change.

Other defects

• finish() failure still publishes the path. on_message_complete sets req.body_file_path (parser.h:124-125) before checking ok (:126), so global after-handlers on the 500 see a path to a closed, possibly truncated file that parser_.clear() unlinks moments later. Moot with the handle design; otherwise set the path only on success.
• The file is unlinked when the response starts streaming, not after it is sent. Cleanup is tied to parser_.clear(), which do_write_sync() runs after every write (http_connection.h:548); for a response at or above res_stream_threshold_ that is the first 16 KiB chunk. With a 2 MiB response the spool directory was already empty while the client was still receiving the body [measured]. The guide says "deleted after the response is sent". Harmless for a synchronous handler that has finished with the file; wrong for anything that reads it later, and on Windows remove() on a still-open file fails silently (body_sink.h:90-93). Either document the actual timing or move body cleanup to response completion.
• Directory creation at builder time. .body_file(dir) (routing.h:662-666) and app.body_file_directory() (app.h:556-560) call create_directories when the route or app is configured, relative to the cwd of that moment, with the error discarded; the sink then opens against the cwd at request time. A service that daemonises after setup turns every upload into a 500 with no log line. Resolve to an absolute path once, require the directory to exist, and fail loudly.
• temp_directory_path() per request (body_sink.h:45): an environment lookup and a stat on every upload; resolve once in the factory.
• The example returns 500 on an empty body (examples/example_body_file.cpp:16-17, same snippet in the guide). Because the sink is only created when a body is coming, a .body_file() route sees has_body_file() == false for an empty PUT; that is a legitimate request and the example should handle it, or the guide should state the two-mode contract plainly.
• An empty chunked body does create a file (0\r\n\r\n gives has_body_file() == true, size 0 [measured]), while Content-Length: 0 does not; consistent with the framing flags, just document it.
• Three copies of the rule lookup (routing.h:1897-1946); one private matched_rule(found) helper, or the routing_handle_result::rule field suggested on Add a request body size limit that closes over-limit connections #1234, replaces them all.
• body_file_, body_file_directory_ and body_sink_factory_ sit on every BaseRule, including WebSocketRule and the static-file rules that can never use them; with the file expressed as a factory only the factory remains.
• The PR text mentions keep_body_file(), which does not exist in the tree; the code and guide say take_body_file().
• Branch shape. The merge commit 71c9f86ba is stacking noise; rebase linearly on Add a request body size limit that closes over-limit connections #1234's head once that lands, and squash the last-call-wins fix into the first commit.

Tests

The 800-line file covers real ground: memory versus file versus custom sink, multi-read, binary, empty, chunked, Expect, keep-alive, 413 cleanup, disconnect cleanup, open/write/finish failure, last-call-wins, concurrent distinct paths, 0600. Missing: a throwing write() and finish(); a sink failure with a body larger than one read (the current failure bodies are 4 bytes, so the reset case above is never exercised); take_body_file() on a copied request; a deferred response holding the file; a response above res_stream_threshold_; a request with no Host; a Blueprint route; HEAD; a chunked over-limit on a sink route (the current 413 test refuses at headers and never opens a file).

Thanks for the rework; the descriptor handling and the failure-to-500 paths are exactly what #1233 lacked, and the sink shape is right. The remaining work is packaging and the boundary around user code, and it removes API rather than adding it.

[anton-n-petrov]

Pushed cffeec895 (the rework you asked for) plus 0b3a6dc0a (a follow-up review's fixes). Replying point by point.

The sink contract

1. Exceptions from write()/finish() — both now wrapped in catch (...) in parser.h, mapped to the 500 path.
2. Handler getting its sink back — request::body_sink is now a shared_ptr<BodySink>; every copy of the request shares it. FileBodySink::from(req) gives the concrete type back.
3. take_body_file() copy trap — gone. Ownership is FileBodySink::keep() on the handle from from(); a copied request keeps the file alive as long as any copy holds the shared_ptr. Test: "keep() survives a copied request".
4. RTTI-free builds — FileBodySink/dynamic_cast moved out to opt-in crow/file_body_sink.h, not included by crow.h/parser.h. BodySink itself has no RTTI and no OS headers.
5. Small gaps — remote_ip_address is now set before the factory call; a nullptr factory result means "keep the body in req.body", not a 500; body-file.md now states the sink-vs-multipart caveat explicitly; sink write/finish failures share the same linger_close() as the 413 (independently implemented on this branch, ahead of Add a request body size limit that closes over-limit connections #1234's version at the time — not yet reconciled with it).

Your sketch under "What I would merge instead" is close to what landed: BodySink{write, finish}, BodySinkFactory, .body_sink(...) on the route, request::body_sink as the shared_ptr, FileBodySink::factory()/from()/path()/keep() in the opt-in header. One deliberate deviation: POSIX file creation uses open(O_CREAT|O_EXCL|O_WRONLY|O_CLOEXEC) + a random name (same scheme as the Windows branch) instead of mkostemp. A follow-up review found mkostemp is glibc-only — unavailable on macOS/BSD and older musl — which breaks the opt-in header's one job. Traded your measured perf point (constructing random_device/mt19937 per attempt) for portability, since this runs once per upload, not per request.

Other defects

• Cleanup timing — fixed structurally: since the handler doesn't run on a failed finish(), there's no path to a stale path(); and since cleanup now tracks the last copy of the request rather than parser_.clear(), a response that outlives the write no longer loses the file mid-stream. Test: "deleted only after the full response is sent".
• Directory resolution — resolve_directory() runs once in factory(), resolves to an absolute path, and throws if the directory doesn't exist rather than creating it.
• temp_directory_path() — resolved once in factory(), not per request.
• Empty-body example — example_body_file.cpp already handles FileBodySink::from(req) == nullptr (empty PUT) with a 0-byte reply.
• Empty chunked body vs. Content-Length: 0 — still undocumented as far as I can tell; not chased further this round.
• Three copies of the rule lookup — consolidated into one matched_rule() helper in routing.h, per your first suggested option.
• Per-rule body_file_/body_file_directory_/body_sink_factory_ fields — down to the one body_sink_factory_ field; .body_file() and app.body_file_directory() are gone, crow::FileBodySink::factory(dir) is the only entry point.
• PR description — still describes the old .body_file()/take_body_file() API; leaving that update to the author since it's user-facing text, not code.
• Branch shape — still stacked on the 71c9f86 merge commit; per the PR description, that's meant to resolve once Add a request body size limit that closes over-limit connections #1234 lands and this rebases onto it, so left as-is for now.

Tests: added for a throwing write()/finish(), a sink failure on a body larger than one read, keep() on a copied request, chunked over-limit on a sink route, factory returning nullptr, a throwing factory, FileBodySink::factory on a missing directory, and concurrent uploads. Not seeing coverage yet for a Blueprint route, a request with no Host, or HEAD on a sink route. The shared TestClient header is still duplicated with max_body_size_tests.cpp.

Separately from this review, a later pass caught two more issues also fixed in 0b3a6dc: docs/guides/app.md still said an over-limit body "is not read or drained" (stale once linger_close() landed), and the mkostemp portability issue above.

Full suite passes: 1132 assertions, 150 test cases.

[ssubbotin]

This reintroduces the allocation from an attacker-controlled Content-Length that the current #1234 head removed. With a large finite max_body_size, a client can send headers only and make every connection reserve the full advertised size before any body byte arrives, so concurrent requests can still exhaust memory. Please update this branch onto the current #1234 head and keep the unconditional removal of this reserve() path, together with req_.body.clear() on rejection.

[ssubbotin]

The amalgamated build violates this opt-in guarantee. scripts/merge_all.py globs every include/crow/*.h*, so file_body_sink.h is copied into crow_all.h; compiling that generated header with -fno-rtti -DASIO_NO_TYPEID then fails at the dynamic_cast in FileBodySink::from(). Please keep this header out of the amalgamated output, or remove its RTTI dependency, and add a generated-header compilation test for this configuration.

[ssubbotin]

Please refresh the PR title and description after the API rewrite. They still advertise .body_file(), app.body_file_directory(), req.body_file_path, take_body_file(), and keep_body_file(), all of which were removed. The current API uses .body_sink(crow::FileBodySink::factory(...)), FileBodySink::from(), path(), and keep(). The testing summary should be updated as well. docs/guides/body-file.md also still says POSIX uses mkostemp, while the current implementation uses an open(O_CREAT | O_EXCL) retry loop.

[ssubbotin]

[P2] This branch still clears the parser while a body-error callback is on the http_parser_execute stack. The small-response 413/500 path calls do_write_sync() with clear_parser == true from on_body or on_message_complete, and this branch lacks the guard added to #1234 in 17f1a187a. Please preserve that fix when rebasing: skip parser_.clear() while body_error_status_ is set. The current #1234 and #1235 heads also have five merge conflicts, so this needs explicit reconciliation.

[ssubbotin]

[P2] The timing contract is incorrect. Only the factory runs at headers-complete before 100 Continue; write() runs after the body starts arriving, so an Expect: 100-continue request receives the interim 100 before a later write() failure can produce 500, and finish() runs at message-complete. Please distinguish factory timing from write()/finish() here and in docs/guides/body-file.md.