Keep the error body of a failed streaming request, and fix the three framing defects the review found

mcpp.toml

@@ -1,7 +1,7 @@
 [package]
 namespace   = "mcpplibs"
 name        = "tinyhttps"
-version     = "0.2.9"
+version     = "0.2.10"
 description = "Minimal C++23 HTTP/HTTPS client with SSE streaming support"
 license     = "Apache-2.0"
 repo        = "https://github.com/mcpplibs/tinyhttps"

src/http.cppm

@@ -242,6 +242,37 @@ static int parse_hex(std::string_view s) {
     return result;
 }
 
+// `Content-Length`, rejected rather than salvaged.
+//
+// Both readers parsed this by walking the characters and keeping the digits, so
+// `12abc` was 12, `abc` was 0 — indistinguishable from a genuine `0` — and a
+// value past the width of the accumulator wrapped in silence. A length is the
+// number of bytes the reader will then trust, so a wrong one is not a cosmetic
+// error: too small leaves the next response's bytes in the stream, and too
+// large waits for bytes that are not coming.
+//
+// Shaped like `parse_chunk_size_line` above, and exported for the same reason:
+// it is the half of the body framing that can be examined without a server.
+export std::optional<std::int64_t>
+parse_content_length(std::string_view value) {
+    // A field value may carry optional whitespace on either side (RFC 9110).
+    while (!value.empty() && (value.front() == ' ' || value.front() == '\t'))
+        value.remove_prefix(1);
+    while (!value.empty() && (value.back() == ' ' || value.back() == '\t'))
+        value.remove_suffix(1);
+    if (value.empty()) return std::nullopt;
+
+    std::uint64_t parsed {};
+    auto [end, error] = std::from_chars(
+        value.data(), value.data() + value.size(), parsed, 10);
+    if (error != std::errc{} || end != value.data() + value.size()
+        || parsed > static_cast<std::uint64_t>(
+            std::numeric_limits<std::int64_t>::max())) {
+        return std::nullopt;
+    }
+    return static_cast<std::int64_t>(parsed);
+}
+
 export std::optional<std::int64_t>
 parse_chunk_size_line(std::string_view line) {
     if (line.empty()) return std::nullopt;
@@ -256,6 +287,25 @@ parse_chunk_size_line(std::string_view line) {
     return static_cast<std::int64_t>(value);
 }
 
+// A streaming request that fails is answered with an error document, not an
+// event stream, so SseParser finds no event boundary in it and yields nothing.
+// The bytes are still worth keeping: without them a caller can report the
+// status line but never the reason. Bounded, so a server answering 5xx with an
+// endless body cannot grow the buffer without limit.
+export inline constexpr std::size_t stream_error_body_limit = 1024 * 1024;
+
+// Appends as much of `data` as `limit` still allows. Returns false once the
+// buffer is full, so a caller can stop copying without tracking sizes itself.
+export bool append_within_limit(std::string& buffer, std::string_view data,
+                                std::size_t limit) {
+    if (buffer.size() >= limit) return false;
+    const std::size_t room = limit - buffer.size();
+    // Not std::min: <winsock2.h> defines a `min` macro on Windows.
+    const std::size_t take = data.size() < room ? data.size() : room;
+    buffer.append(data.substr(0, take));
+    return take == data.size();
+}
+
 // Case-insensitive string comparison
 static bool iequals(std::string_view a, std::string_view b) {
     if (a.size() != b.size()) return false;
@@ -459,12 +509,11 @@ private:
                     chunked = true;
                 }
                 if (iequals(key, "Content-Length")) {
-                    contentLength = 0;
-                    for (char c : valStr) {
-                        if (c >= '0' && c <= '9') {
-                            contentLength = contentLength * 10 + (c - '0');
-                        }
-                    }
+                    // Rejected rather than salvaged; parse_content_length says
+                    // why. A malformed value leaves this at -1, which is the
+                    // same state as an absent header and is a framing this
+                    // reader already handles.
+                    contentLength = parse_content_length(valStr).value_or(-1);
                 }
                 if (iequals(key, "Connection") && iequals(valStr, "close")) {
                     connectionClose = true;
@@ -704,6 +753,7 @@ public:
         // Read headers
         bool chunked = false;
         bool connectionClose = false;
+        std::int64_t contentLength = -1;
 
         while (true) {
             std::string headerLine = read_line(*sock, config_.readTimeoutMs);
@@ -726,14 +776,27 @@ public:
                 if (iequals(key, "Connection") && iequals(valStr, "close")) {
                     connectionClose = true;
                 }
+                // send() reads this and send_stream() did not, which is the
+                // same asymmetry this change exists to remove. See the body
+                // loop below for what its absence cost.
+                if (iequals(key, "Content-Length")) {
+                    contentLength = parse_content_length(valStr).value_or(-1);
+                }
             }
         }
 
         // Stream body incrementally, feeding chunks to SseParser
         SseParser parser;
         bool stopped = false;
+        // send() fills `body` on every path including failures; without this
+        // send_stream would be the one entry point that drops it. Capturing is
+        // additive — events are still parsed and dispatched exactly as before.
+        const bool captureBody = !response.ok();
 
         auto dispatch = [&](std::string_view data) -> bool {
+            if (captureBody) {
+                append_within_limit(response.body, data, stream_error_body_limit);
+            }
             auto events = parser.feed(data);
             for (const auto& ev : events) {
                 if (!callback(ev)) {
@@ -756,7 +819,22 @@ public:
                     sizeLine.pop_back();
                 }
 
-                int chunkSize = parse_hex(sizeLine);
+                // A CHUNK HEADER THAT DOES NOT PARSE IS NOT A TERMINAL CHUNK.
+                //
+                // `parse_hex` returned what it had accumulated when it met a
+                // character it did not recognise, and zero for an empty line —
+                // and `read_line` returns an empty line on a timeout or a
+                // closed connection. So a stream that was cut short read as a
+                // stream that ended cleanly, and this loop reported success.
+                // #9 established `parse_chunk_size_line` for exactly this and
+                // it reached only `download_to_file`.
+                auto parsedChunkSize = parse_chunk_size_line(sizeLine);
+                if (!parsedChunkSize) {
+                    response.statusText = "Invalid chunk size: " + sizeLine;
+                    connectionClose = true;
+                    break;
+                }
+                int chunkSize = static_cast<int>(*parsedChunkSize);
                 if (chunkSize == 0) {
                     // Terminal chunk — read trailing \r\n
                     read_line(*sock, config_.readTimeoutMs);
@@ -775,8 +853,44 @@ public:
                     break;
                 }
             }
+        } else if (contentLength >= 0) {
+            // A DECLARED LENGTH IS READ AND THE READER THEN STOPS.
+            //
+            // This branch did not exist: a response that was not chunked was
+            // read until the connection closed, whatever its headers said. On
+            // the library's own defaults — `keepAlive = true`, so the request
+            // carries `Connection: keep-alive` — the server does not close,
+            // and the loop below ran until `readTimeoutMs` expired.
+            //
+            // Measured against httpbin's `/status/418`, which answers with a
+            // Content-Length and keeps the connection:
+            //
+            //     keepAlive = false   status=418 body=135   elapsed  1370 ms
+            //     keepAlive = true    status=418 body=135   elapsed  9379 ms   (readTimeoutMs = 8000)
+            //
+            // The error body arrived either way, and on the defaults it arrived
+            // a full read timeout late — sixty seconds, as the defaults stand.
+            // `send()` has had this branch throughout, which is why the same
+            // request through it returns at once.
+            std::int64_t remaining = contentLength;
+            char buf[4096];
+            while (!stopped && remaining > 0) {
+                if (!sock->wait_readable(config_.readTimeoutMs)) {
+                    break;
+                }
+                const auto want = static_cast<std::size_t>(
+                    remaining < static_cast<std::int64_t>(sizeof buf)
+                        ? remaining : static_cast<std::int64_t>(sizeof buf));
+                int ret = sock->read(buf, want);
+                if (ret <= 0) break;
+                remaining -= ret;
+                if (!dispatch(std::string_view(buf, static_cast<std::size_t>(ret)))) {
+                    break;
+                }
+            }
         } else {
-            // Not chunked — read until connection closes
+            // Neither chunked nor a declared length: the end of the body is the
+            // end of the connection, so the connection must not be reused.
             connectionClose = true;
             char buf[4096];
             while (!stopped) {

tests/test_download.cpp

@@ -20,6 +20,75 @@ TEST(ChunkedProtocol, AcceptsValidSizeAndTerminalChunk) {
     EXPECT_EQ(*https::parse_chunk_size_line("0"), 0);
 }
 
+// `Content-Length` decides how many bytes a reader will trust, so a wrong
+// answer is not cosmetic: too small leaves the next response's bytes in the
+// stream and too large waits for bytes that are not coming.
+//
+// Both readers used to keep the digits and discard everything else. Measured,
+// by compiling that parser on its own:
+//
+//     "135"                  -> 135
+//     "0"                    -> 0
+//     "abc"                  -> 0                     <- a refusal read as a real zero
+//     "12abc"                -> 12                    <- stops twelve bytes in
+//     ""                     -> 0
+//     "-1"                   -> 1                     <- the sign is discarded
+//     "99999999999999999999" -> 7766279631452241919   <- wraps, in silence
+//
+// The last two are the ones no amount of care at the call site could recover
+// from, because the value it receives is a plausible number.
+TEST(ContentLength, AcceptsAWellFormedValue) {
+    ASSERT_TRUE(https::parse_content_length("135").has_value());
+    EXPECT_EQ(*https::parse_content_length("135"), 135);
+    // A field value may carry optional whitespace on either side.
+    ASSERT_TRUE(https::parse_content_length("  135\t").has_value());
+    EXPECT_EQ(*https::parse_content_length("  135\t"), 135);
+}
+
+// The one a salvaging parser cannot express. `abc` used to yield 0, which is
+// indistinguishable from a server that genuinely declared an empty body — and
+// the two call for opposite behaviour.
+TEST(ContentLength, AZeroIsDistinguishableFromARefusal) {
+    ASSERT_TRUE(https::parse_content_length("0").has_value());
+    EXPECT_EQ(*https::parse_content_length("0"), 0);
+    EXPECT_FALSE(https::parse_content_length("abc").has_value());
+}
+
+TEST(ContentLength, RejectsEmptyTrailingGarbageAndOverflow) {
+    EXPECT_FALSE(https::parse_content_length("").has_value());
+    EXPECT_FALSE(https::parse_content_length("   ").has_value());
+    // `12abc` used to be 12: the reader would then stop twelve bytes in and
+    // leave the rest of the body to be read as the next response.
+    EXPECT_FALSE(https::parse_content_length("12abc").has_value());
+    EXPECT_FALSE(https::parse_content_length("-1").has_value());
+    EXPECT_FALSE(https::parse_content_length("+1").has_value());
+    // Past the width of the accumulator, which used to wrap in silence.
+    EXPECT_FALSE(https::parse_content_length("99999999999999999999").has_value());
+}
+
+TEST(StreamErrorBody, KeepsEverythingWhileUnderLimit) {
+    std::string buffer;
+    EXPECT_TRUE(https::append_within_limit(buffer, "abc", 8));
+    EXPECT_TRUE(https::append_within_limit(buffer, "de", 8));
+    EXPECT_EQ(buffer, "abcde");
+}
+
+TEST(StreamErrorBody, TruncatesTheChunkThatCrossesTheLimit) {
+    std::string buffer = "abc";
+    EXPECT_FALSE(https::append_within_limit(buffer, "defgh", 5));
+    EXPECT_EQ(buffer, "abcde");
+}
+
+TEST(StreamErrorBody, RefusesFurtherDataOnceFull) {
+    std::string buffer = "abcde";
+    EXPECT_FALSE(https::append_within_limit(buffer, "f", 5));
+    EXPECT_EQ(buffer, "abcde");
+    // A zero limit must not append anything, not even an empty append.
+    std::string empty;
+    EXPECT_FALSE(https::append_within_limit(empty, "a", 0));
+    EXPECT_TRUE(empty.empty());
+}
+
 TEST(DownloadResultContract, CarriesTransferAndResponseMetadata) {
     https::DownloadToFileResult result;
     result.bytesWritten = 42;
@@ -36,6 +105,86 @@ TEST(DownloadResultContract, CarriesTransferAndResponseMetadata) {
     EXPECT_FALSE(result.lastModified.empty());
 }
 
+// Test that a failed streaming request carries its error body, against a real
+// HTTPS endpoint. httpbin's /status/418 answers non-2xx with a body, which is
+// exactly the shape SseParser cannot turn into events.
+
+class StreamErrorBodyLiveTest : public ::testing::Test {
+protected:
+    void SetUp() override { https::Socket::platform_init(); }
+};
+
+// ON THE LIBRARY'S OWN DEFAULTS, AND THE READ TIMEOUT IS PART OF THE
+// OBSERVATION RATHER THAN A SAFETY NET.
+//
+// The first form of this test set `keepAlive = false`, "so the server closes
+// and the read loop ends". That comment was the defect: `send_stream` had no
+// branch for a declared `Content-Length`, so a response that was not chunked
+// was read until the connection closed — and on the defaults the server does
+// not close. The body arrived, one full `readTimeoutMs` late.
+//
+//     keepAlive = false   status=418 body=135   elapsed  1370 ms
+//     keepAlive = true    status=418 body=135   elapsed  9379 ms   (timeout 8000)
+//
+// So the configuration under test is the default one, and the elapsed time is
+// asserted. A test that closed the connection to make the loop end would be
+// examining the one arrangement in which the defect does not appear.
+TEST_F(StreamErrorBodyLiveTest, FailedStreamKeepsTheErrorBody) {
+    https::HttpClientConfig cfg;
+    cfg.connectTimeoutMs = 15000;
+    cfg.readTimeoutMs = 30000;
+    // keepAlive is left at its default, which is true.
+    https::HttpClient client(cfg);
+
+    https::HttpRequest req;
+    req.method = https::Method::GET;
+    req.url = "https://httpbin.org/status/418";
+
+    int events = 0;
+    const auto started = std::chrono::steady_clock::now();
+    auto res = client.send_stream(req, [&](const https::SseEvent&) {
+        ++events;
+        return true;
+    });
+    const auto elapsed = std::chrono::duration_cast<std::chrono::milliseconds>(
+        std::chrono::steady_clock::now() - started).count();
+
+    EXPECT_EQ(res.statusCode, 418);
+    EXPECT_EQ(events, 0) << "an error document is not an event stream";
+    EXPECT_FALSE(res.body.empty()) << "error body was dropped";
+    EXPECT_NE(res.body.find("teapot"), std::string::npos);
+    // Generous against a slow runner and still an order of magnitude below the
+    // read timeout, which is what the defect consumed.
+    EXPECT_LT(elapsed, 15000)
+        << "the body arrived after " << elapsed
+        << " ms; a declared Content-Length was not honoured and the reader "
+           "waited for a close that keep-alive was never going to bring";
+}
+
+// The success path, on the framing an event stream actually uses. A 2xx is not
+// captured, so `body` stays empty and the bytes reach the parser — and it must
+// still return promptly, since the chunked branch is the one this change did
+// not restructure.
+TEST_F(StreamErrorBodyLiveTest, AChunkedSuccessIsUnchangedAndPrompt) {
+    https::HttpClientConfig cfg;
+    cfg.connectTimeoutMs = 15000;
+    cfg.readTimeoutMs = 30000;
+    https::HttpClient client(cfg);
+
+    https::HttpRequest req;
+    req.method = https::Method::GET;
+    req.url = "https://httpbin.org/stream/3";
+
+    const auto started = std::chrono::steady_clock::now();
+    auto res = client.send_stream(req, [](const https::SseEvent&) { return true; });
+    const auto elapsed = std::chrono::duration_cast<std::chrono::milliseconds>(
+        std::chrono::steady_clock::now() - started).count();
+
+    EXPECT_EQ(res.statusCode, 200);
+    EXPECT_TRUE(res.body.empty()) << "a 2xx body is not captured; it is the caller's stream";
+    EXPECT_LT(elapsed, 15000) << "the chunked reader did not terminate promptly";
+}
+
 // Test download_to_file against a real HTTPS endpoint.
 // Uses httpbin.org which returns known-size responses.