Progressive timeout dev - #3409
Open
zchuango wants to merge 2 commits into
Open
Conversation
Contributor
Author
|
@wwbmmm @chenBright This feature PR 3163 hadn't been merged into the community before. Recently, there was a conflict during master code synchronization, so I resubmitted the PR using a new branch. I hope it will be reviewed again, looking forward to this feature being merged into the community. |
wwbmmm
reviewed
Jul 30, 2026
| s->parsing_context()->Destroy(); | ||
| } | ||
| s->ReleaseReferenceIfIdle(0); | ||
| } |
| #include "brpc/grpc.h" | ||
| #include "brpc/kvmap.h" | ||
| #include "brpc/rpc_dump.h" | ||
|
|
| uint64_t log_id; | ||
| std::string request_id; | ||
| }; | ||
|
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds support for timing out progressive HTTP response body reads when the reader becomes idle, aiming to close the underlying client socket when no new progressive data arrives within a configured duration.
Changes:
- Introduces a
progressive_read_timeout_mssetting onbrpc::Controllerand wrapsProgressiveReaderwith a timeout-monitoring reader. - Plumbs the HTTP
SocketIdinto the progressive-attachment context so the timeout logic can act on the underlying connection. - Updates the
http_c++example server/client to demonstrate and manually exercise progressive read timeout behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/brpc/progressive_reader.h | Extends progressive attachment interface to expose a SocketId for timeout/connection handling. |
| src/brpc/policy/http_rpc_protocol.h | Stores the socket id on HttpContext to support timeout actions. |
| src/brpc/policy/http_rpc_protocol.cpp | Sets the socket id on newly created HTTP parsing contexts. |
| src/brpc/errno.proto | Adds a new errno value for progressive read timeout. |
| src/brpc/controller.h | Adds the controller API surface for configuring progressive read timeout. |
| src/brpc/controller.cpp | Implements the timeout wrapper logic and the timeout-triggered connection handling. |
| example/http_c++/http_server.cpp | Adds an example switch to simulate stalled progressive writes. |
| example/http_c++/http_client.cpp | Adds an example progressive reader and CLI flags to exercise timeout behavior. |
Comments suppressed due to low confidence (2)
src/brpc/controller.cpp:203
- ProgressiveTimeoutReader is allocated with new() in Controller::ReadProgressiveAttachmentBy(), but it never deletes itself. Also, canceling the timer only after calling the underlying reader's OnEndOfMessage risks the timer firing while OnEndOfMessage blocks. Cancel the timer first, then forward the status, then delete this wrapper.
void OnEndOfMessage(const butil::Status& status) {
if (_is_read_timeout) {
_reader->OnEndOfMessage(butil::Status(EPROGREADTIMEOUT, "The progressive read timeout"));
} else {
_reader->OnEndOfMessage(status);
example/http_c++/http_client.cpp:55
- PartDataReader is allocated with new in main(), but it never deletes itself, causing a leak in the example. ProgressiveReader implementations typically delete themselves in OnEndOfMessage once they are done.
void OnEndOfMessage(const butil::Status& status) {
_done->signal();
LOG(INFO) << "progressive read data final status : " << status;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
22
to
+23
| #include "brpc/shared_object.h" | ||
| #include "brpc/socket.h" |
Comment on lines
100
to
104
| : InputMessageBase() | ||
| , HttpMessage(read_body_progressively, request_method) | ||
| , _is_stage2(false) { | ||
| , _is_stage2(false) | ||
| , _socket_id(0) { | ||
| // add one ref for Destroy |
Comment on lines
+195
to
+197
| butil::Status OnReadOnePart(const void* data, size_t length) { | ||
| return _reader->OnReadOnePart(data, length); | ||
| } |
Comment on lines
+225
to
+226
| LOG_IF(INFO, log_idle) << "progressive read timeout socket id : " << reader->_socket_id | ||
| << " progressive read timeout us : " << reader->_read_timeout_ms; |
Comment on lines
+227
to
+230
| if (s->parsing_context() != NULL) { | ||
| s->parsing_context()->Destroy(); | ||
| } | ||
| s->ReleaseReferenceIfIdle(0); |
Comment on lines
+415
to
+422
| void Controller::set_progressive_read_timeout_ms(int32_t progressive_read_timeout_ms){ | ||
| if(progressive_read_timeout_ms <= 0x7fffffff){ | ||
| _progressive_read_timeout_ms = progressive_read_timeout_ms; | ||
| } else { | ||
| _progressive_read_timeout_ms = 0x7fffffff; | ||
| LOG(WARNING) << "progressive_read_timeout_seconds is limited to 0x7fffffff"; | ||
| } | ||
| } |
Comment on lines
+1699
to
+1702
| if (progressive_read_timeout_ms() > 0) { | ||
| auto reader = new ProgressiveTimeoutReader(_rpa->GetSocketId(), _progressive_read_timeout_ms, r); | ||
| return _rpa->ReadProgressiveAttachmentBy(reader); | ||
| } |
| ESSL = 1016; // SSL related error | ||
| EH2RUNOUTSTREAMS = 1017; // The H2 socket was run out of streams | ||
| EREJECT = 1018; // The Request is rejected | ||
| EPROGREADTIMEOUT = 1019; // The Progressive read timeout |
Comment on lines
+46
to
+50
| butil::Status OnReadOnePart(const void* data, size_t length) { | ||
| memcpy(_buffer, data, length); | ||
| LOG(INFO) << "data ： " << _buffer << " size : " << length; | ||
| return butil::Status::OK(); | ||
| } |
| DEFINE_string(certificate, "cert.pem", "Certificate file path to enable SSL"); | ||
| DEFINE_string(private_key, "key.pem", "Private key file path to enable SSL"); | ||
| DEFINE_string(ciphers, "", "Cipher suite used for SSL connections"); | ||
| DEFINE_bool(enable_progressive_timeout, false, "whether or not trigger progressive write attachement data timeout"); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: resolve #3133 PR Number: #3163
Problem Summary: when Controller response_will_be_read_progressively() it needs background bthread monitor handler to solve progressive reader idle timeout，when hit the progressive reader idle timeout duration should close current client socket connection.
What is changed and the side effects?
Changed:
Side effects:
Performance effects: NO
Breaking backward compatibility: NO
Check List: