Skip to content

Commit a31e413

Browse files
committed
fix(http): keep the error body of a failed streaming request
send() fills HttpResponse::body on every path including failures; send_stream() was the one entry point that dropped it. A non-2xx answer to a streaming request is an error document, not an event stream: SseParser finds no event boundary in it, emits nothing, and the bytes stay in its private buffer. Callers were left with a status line and no reason. Capture the body when the status is not 2xx. Events are still parsed and dispatched exactly as before, and nothing is copied on a 2xx stream, so the success path is byte-identical. The copy is bounded by stream_error_body_limit (1 MiB) so a server answering 5xx with an endless body cannot grow the buffer without limit. Truncation lives in an exported append_within_limit, in the same spirit as parse_chunk_size_line, with three unit tests for under, across and past the limit; a live test against httpbin's /status/418 covers the wiring.
1 parent 965d805 commit a31e413

3 files changed

Lines changed: 82 additions & 1 deletion

File tree

mcpp.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
[package]
22
namespace = "mcpplibs"
33
name = "tinyhttps"
4-
version = "0.2.9"
4+
version = "0.2.10"
55
description = "Minimal C++23 HTTP/HTTPS client with SSE streaming support"
66
license = "Apache-2.0"
77
repo = "https://github.com/mcpplibs/tinyhttps"

src/http.cppm

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -256,6 +256,25 @@ parse_chunk_size_line(std::string_view line) {
256256
return static_cast<std::int64_t>(value);
257257
}
258258

259+
// A streaming request that fails is answered with an error document, not an
260+
// event stream, so SseParser finds no event boundary in it and yields nothing.
261+
// The bytes are still worth keeping: without them a caller can report the
262+
// status line but never the reason. Bounded, so a server answering 5xx with an
263+
// endless body cannot grow the buffer without limit.
264+
export inline constexpr std::size_t stream_error_body_limit = 1024 * 1024;
265+
266+
// Appends as much of `data` as `limit` still allows. Returns false once the
267+
// buffer is full, so a caller can stop copying without tracking sizes itself.
268+
export bool append_within_limit(std::string& buffer, std::string_view data,
269+
std::size_t limit) {
270+
if (buffer.size() >= limit) return false;
271+
const std::size_t room = limit - buffer.size();
272+
// Not std::min: <winsock2.h> defines a `min` macro on Windows.
273+
const std::size_t take = data.size() < room ? data.size() : room;
274+
buffer.append(data.substr(0, take));
275+
return take == data.size();
276+
}
277+
259278
// Case-insensitive string comparison
260279
static bool iequals(std::string_view a, std::string_view b) {
261280
if (a.size() != b.size()) return false;
@@ -732,8 +751,15 @@ public:
732751
// Stream body incrementally, feeding chunks to SseParser
733752
SseParser parser;
734753
bool stopped = false;
754+
// send() fills `body` on every path including failures; without this
755+
// send_stream would be the one entry point that drops it. Capturing is
756+
// additive — events are still parsed and dispatched exactly as before.
757+
const bool captureBody = !response.ok();
735758

736759
auto dispatch = [&](std::string_view data) -> bool {
760+
if (captureBody) {
761+
append_within_limit(response.body, data, stream_error_body_limit);
762+
}
737763
auto events = parser.feed(data);
738764
for (const auto& ev : events) {
739765
if (!callback(ev)) {

tests/test_download.cpp

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,29 @@ TEST(ChunkedProtocol, AcceptsValidSizeAndTerminalChunk) {
2020
EXPECT_EQ(*https::parse_chunk_size_line("0"), 0);
2121
}
2222

23+
TEST(StreamErrorBody, KeepsEverythingWhileUnderLimit) {
24+
std::string buffer;
25+
EXPECT_TRUE(https::append_within_limit(buffer, "abc", 8));
26+
EXPECT_TRUE(https::append_within_limit(buffer, "de", 8));
27+
EXPECT_EQ(buffer, "abcde");
28+
}
29+
30+
TEST(StreamErrorBody, TruncatesTheChunkThatCrossesTheLimit) {
31+
std::string buffer = "abc";
32+
EXPECT_FALSE(https::append_within_limit(buffer, "defgh", 5));
33+
EXPECT_EQ(buffer, "abcde");
34+
}
35+
36+
TEST(StreamErrorBody, RefusesFurtherDataOnceFull) {
37+
std::string buffer = "abcde";
38+
EXPECT_FALSE(https::append_within_limit(buffer, "f", 5));
39+
EXPECT_EQ(buffer, "abcde");
40+
// A zero limit must not append anything, not even an empty append.
41+
std::string empty;
42+
EXPECT_FALSE(https::append_within_limit(empty, "a", 0));
43+
EXPECT_TRUE(empty.empty());
44+
}
45+
2346
TEST(DownloadResultContract, CarriesTransferAndResponseMetadata) {
2447
https::DownloadToFileResult result;
2548
result.bytesWritten = 42;
@@ -36,6 +59,38 @@ TEST(DownloadResultContract, CarriesTransferAndResponseMetadata) {
3659
EXPECT_FALSE(result.lastModified.empty());
3760
}
3861

62+
// Test that a failed streaming request carries its error body, against a real
63+
// HTTPS endpoint. httpbin's /status/418 answers non-2xx with a body, which is
64+
// exactly the shape SseParser cannot turn into events.
65+
66+
class StreamErrorBodyLiveTest : public ::testing::Test {
67+
protected:
68+
void SetUp() override { https::Socket::platform_init(); }
69+
};
70+
71+
TEST_F(StreamErrorBodyLiveTest, FailedStreamKeepsTheErrorBody) {
72+
https::HttpClientConfig cfg;
73+
cfg.connectTimeoutMs = 15000;
74+
cfg.readTimeoutMs = 30000;
75+
cfg.keepAlive = false; // so the server closes and the read loop ends
76+
https::HttpClient client(cfg);
77+
78+
https::HttpRequest req;
79+
req.method = https::Method::GET;
80+
req.url = "https://httpbin.org/status/418";
81+
82+
int events = 0;
83+
auto res = client.send_stream(req, [&](const https::SseEvent&) {
84+
++events;
85+
return true;
86+
});
87+
88+
EXPECT_EQ(res.statusCode, 418);
89+
EXPECT_EQ(events, 0) << "an error document is not an event stream";
90+
EXPECT_FALSE(res.body.empty()) << "error body was dropped";
91+
EXPECT_NE(res.body.find("teapot"), std::string::npos);
92+
}
93+
3994
// Test download_to_file against a real HTTPS endpoint.
4095
// Uses httpbin.org which returns known-size responses.
4196

0 commit comments

Comments
 (0)