Fix handling of HTTP server request overloads#2236
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug in the HTTP server's request handling where the SPSC buffer's return value was misinterpreted, causing all in-flight requests to be killed when the server received too many requests. The fix introduces a new try_push method that properly enforces capacity limits and updates the error handling to only reject the overflowing request while keeping pending requests intact.
Changes:
- Added
try_pushmethod tospsc_bufferthat treats capacity as a hard limit, unlikepushwhich allows exceeding capacity for burst absorption - Updated
http_request_producer_implto usetry_pushinstead ofpushfor proper rejection semantics - Changed error handling from shutting down all in-flight requests to sending a 503 Service Unavailable response only for rejected requests
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| libcaf_core/caf/async/spsc_buffer.hpp | Adds try_push method with hard capacity enforcement and updates documentation to clarify soft vs hard limit behavior |
| libcaf_net/caf/net/http/with.cpp | Switches to try_push and replaces router shutdown with proper 503 error response for rejected requests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| bool try_push(const T& item) { | ||
| lock_type guard{mtx_}; | ||
| CAF_ASSERT(producer_ != nullptr); | ||
| CAF_ASSERT(!flags_.closed); | ||
| if (buf_.size() >= capacity_) | ||
| return false; | ||
| buf_.push_back(item); | ||
| if (buf_.size() == 1 && consumer_) | ||
| consumer_->on_producer_wakeup(); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The new try_push method lacks test coverage. Consider adding unit tests to verify its behavior, especially for edge cases like:
- Attempting to push when the buffer is exactly at capacity
- Attempting to push when the buffer is above capacity (if that's possible through the regular
pushmethod) - Verifying that
try_pushreturns true when there is available capacity - Verifying that the consumer is properly awakened when the buffer transitions from empty to non-empty
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2236 +/- ##
==========================================
- Coverage 71.27% 71.20% -0.07%
==========================================
Files 635 633 -2
Lines 30676 30691 +15
Branches 3327 3328 +1
==========================================
- Hits 21865 21855 -10
- Misses 6935 6958 +23
- Partials 1876 1878 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Sorry for the noise. Closed by mistake while looking at the PR from phone. |
66a1163 to
3f18b9a
Compare
http_request_producer_impl::pushhas a bug: the SPSC buffer returning 0 does not mean the item has been rejected but merely that the capacity reached zeroThis set of changes introduces
try_pushon SPSC buffers that rejects additional items when at or above capacity. Further, the HTTP server now sends an error only in response to the rejected request. Pending requests are unaffected.