Verify HTTP server overload responds with HTTP 503 - #2436
Conversation
f10c859 to
ad18d1d
Compare
There was a problem hiding this comment.
Pull request overview
Adds a regression test to ensure CAF’s HTTP server responds with HTTP 503 when its request queue is overloaded (follow-up to #2236), while still allowing already-queued requests to complete normally.
Changes:
- Introduces low-level HTTP response parsing helper(s) to validate raw responses from a socket pair.
- Adds a new test that overloads the request queue via HTTP pipelining and verifies overflow requests get HTTP 503 while queued requests later return HTTP 200.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2436 +/- ##
==========================================
+ Coverage 72.95% 73.11% +0.15%
==========================================
Files 642 642
Lines 30702 30718 +16
Branches 3362 3364 +2
==========================================
+ Hits 22400 22460 +60
+ Misses 6371 6323 -48
- Partials 1931 1935 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ad18d1d to
df4a351
Compare
df4a351 to
8ae8d86
Compare
| request_buffer->close(); | ||
| net::http::request dummy; | ||
| require_eq(consumer.pull(async::delay_errors, dummy, 1s), | ||
| async::read_result::stop); |
There was a problem hiding this comment.
This will cause an ASAN memory leak described in #2437.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
|
||
| using latch_ptr = std::shared_ptr<std::latch>; | ||
|
|
||
| struct raw_http_response { |
There was a problem hiding this comment.
What's raw about this? It has already been parsed.
| raw_header.push_back( | ||
| static_cast<char>(std::to_integer<unsigned char>(buf[0]))); |
There was a problem hiding this comment.
| raw_header.push_back( | |
| static_cast<char>(std::to_integer<unsigned char>(buf[0]))); | |
| raw_header.push_back(std::to_integer<char>(buf[0])); |
| byte_buffer buf; | ||
| buf.resize(1); |
There was a problem hiding this comment.
| byte_buffer buf; | |
| buf.resize(1); |
| buf.resize(1); | ||
| while (raw_header.find("\r\n\r\n") == std::string::npos) { | ||
| require_readable(); | ||
| auto bytes_read = net::read(fd, buf); |
There was a problem hiding this comment.
We don't need a heap-allocated buffer for a single byte.
| auto bytes_read = net::read(fd, buf); | |
| auto buf = std::byte{}; | |
| auto bytes_read = net::read(fd, std::span{&buf, 1}); |
| while (raw_header.find("\r\n\r\n") == std::string::npos) { | ||
| require_readable(); | ||
| auto bytes_read = net::read(fd, buf); | ||
| self.require_eq(bytes_read, ptrdiff_t{1}, loc); |
There was a problem hiding this comment.
| self.require_eq(bytes_read, ptrdiff_t{1}, loc); | |
| self.require_eq(bytes_read, 1, loc); |
| raw_http_response result; | ||
| auto parse_result = result.header.parse(raw_header); | ||
| self.require_eq(parse_result.first, net::http::status::ok, loc); | ||
| auto content_length = result.header.content_length().value_or(size_t{0}); |
There was a problem hiding this comment.
| auto content_length = result.header.content_length().value_or(size_t{0}); | |
| auto content_length = result.header.content_length().value_or(0u); |
| check_eq(elog->errors(), std::vector<error>{}); | ||
| } | ||
|
|
||
| SCENARIO("server responds with 503 unavailable when queue is overloaded") { |
There was a problem hiding this comment.
| SCENARIO("server responds with 503 unavailable when queue is overloaded") { | |
| SCENARIO("a server responds with status 503 on queue overloads") { |
| auto overflow_count = size_t{2}; | ||
| auto [server_fd, client_fd] = unbox(net::make_stream_socket_pair()); | ||
| net::socket_guard client_guard{client_fd}; | ||
| std::optional<async::consumer_resource<net::http::request>> requests; |
There was a problem hiding this comment.
The resource has a default constructor, can be assigned and has a valid() method. No need to wrap this into an optional.
| std::optional<async::consumer_resource<net::http::request>> requests; | |
| async::consumer_resource<net::http::request> requests; |
| // Fill the queue, and send two requests that overflow the queue. | ||
| for (auto i = size_t{0}; i < request_count; ++i) { | ||
| auto req = detail::format("GET /{} HTTP/1.1\r\nHost: localhost\r\n\r\n", | ||
| i); | ||
| require_eq(net::write(client_fd, as_bytes(std::span{req})), | ||
| static_cast<ptrdiff_t>(req.size())); | ||
| } | ||
| THEN("the two overflow requests receive 503 responses") { | ||
| for (auto i = size_t{0}; i < overflow_count; ++i) { | ||
| auto response = read_http_response(client_fd); | ||
| check_eq(response.header.status(), uint16_t{503}); | ||
| check_eq(response.header.status_text(), "Service Unavailable"); | ||
| check_eq(response.body, | ||
| "service unavailable: request could not be queued"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Am I reading this wrong? We are writing n+2 requests. The last two should overflow. And then the server is sending the errors first?
Isn't all of this using the same socket? The server must process the requests in order. How else can the client match responses to requests? So I would expect n times a valid response and then 2 errors. Per connection, the server cannot start processing a new request before it has replied to the most recent one.
Followup to #2236.