You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by "zchuango (via GitHub)" <gi...@apache.org> on 2026/07/27 03:36:31 UTC
[PR] Progressive timeout dev (brpc)
zchuango opened a new pull request, #3409:
URL: https://github.com/apache/brpc/pull/3409
### 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:
1. add HandleIdleProgressiveReader method will run in and when controller call response_will_be_read_progressively method will trigger HandleIdleProgressiveReader monitor idle progressive reader on bthread_timer.
2. add progressive idle case on http_c++ example http_server with enable_progressive_timeout arg to trigger ProgressiveAttachment write timeout and http_client with progressive and progressive_read_timeout_ms args test the progressive reader idle timeout case.
Side effects:
Performance effects: NO
Breaking backward compatibility: NO
---
### Check List:
- Please make sure your changes are compilable.
- When providing us with a new feature, it is best to add related tests.
- Please follow [Contributor Covenant Code of Conduct](https://github.com/apache/brpc/blob/master/CODE_OF_CONDUCT.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Progressive timeout dev (brpc)
Posted by "zchuango (via GitHub)" <gi...@apache.org>.
zchuango commented on PR #3409:
URL: https://github.com/apache/brpc/pull/3409#issuecomment-5087041571
@wwbmmm @chenBright This feature PR [3163](https://github.com/apache/brpc/pull/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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Progressive timeout dev (brpc)
Posted by "zchuango (via GitHub)" <gi...@apache.org>.
zchuango commented on PR #3409:
URL: https://github.com/apache/brpc/pull/3409#issuecomment-5301437000
Hi @wwbmmm @chenBright,
This PR (#3409) has been open for quite some time. When you have a chance, could you please take another look at the proposed feature?
If there are no remaining concerns, we would greatly appreciate your review and help merging it. If any changes or additional information are needed, please let us know and we’ll follow up promptly.
Thank you for your time and for maintaining the brpc community!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Progressive timeout dev (brpc)
Posted by "zchuango (via GitHub)" <gi...@apache.org>.
zchuango commented on code in PR #3409:
URL: https://github.com/apache/brpc/pull/3409#discussion_r3735816738
##########
src/brpc/policy/http_rpc_protocol.h:
##########
@@ -87,11 +87,20 @@ class HttpContext : public ReadableProgressiveAttachment
, public InputMessageBase
, public HttpMessage {
public:
+ SocketId GetSocketId() override {
+ return _socket_id;
+ }
+
+ void SetSocketId(SocketId id) {
+ _socket_id = id;
+ }
+
explicit HttpContext(bool read_body_progressively,
HttpMethod request_method = HTTP_METHOD_GET)
: InputMessageBase()
, HttpMessage(read_body_progressively, request_method)
- , _is_stage2(false) {
+ , _is_stage2(false)
+ , _socket_id(0) {
// add one ref for Destroy
Review Comment:
Fixed. HttpContext::_socket_id is now initialized to INVALID_SOCKET_ID, so an unset socket ID cannot be mistaken for a valid one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Progressive timeout dev (brpc)
Posted by "zchuango (via GitHub)" <gi...@apache.org>.
zchuango commented on code in PR #3409:
URL: https://github.com/apache/brpc/pull/3409#discussion_r3735936347
##########
example/http_c++/http_server.cpp:
##########
@@ -31,6 +31,7 @@ DEFINE_int32(idle_timeout_s, -1, "Connection will be closed if there is no "
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");
Review Comment:
Fixed. Corrected attachement to attachment in the flag description.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Progressive timeout dev (brpc)
Posted by "zchuango (via GitHub)" <gi...@apache.org>.
zchuango commented on PR #3409:
URL: https://github.com/apache/brpc/pull/3409#issuecomment-5190325438
@wwbmmm Thanks for the follow-up. We are working on addressing the review comments and will update the PR once the changes are ready. Thanks for your patience.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Progressive timeout dev (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on PR #3409:
URL: https://github.com/apache/brpc/pull/3409#issuecomment-5230313703
LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Progressive timeout dev (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3409:
URL: https://github.com/apache/brpc/pull/3409#discussion_r3679786422
##########
src/brpc/progressive_reader.h:
##########
@@ -20,6 +20,7 @@
#define BRPC_PROGRESSIVE_READER_H
#include "brpc/shared_object.h"
+#include "brpc/socket.h"
Review Comment:
Including brpc/socket.h here is unnecessarily heavy for this header; ReadableProgressiveAttachment only needs the SocketId type. Prefer including brpc/socket_id.h to avoid pulling in the full Socket API (reduces compile time and header coupling).
##########
src/brpc/controller.cpp:
##########
@@ -174,6 +175,80 @@ class IgnoreAllRead : public ProgressiveReader {
void OnEndOfMessage(const butil::Status&) {}
};
+class ProgressiveTimeoutReader : public ProgressiveReader {
+public:
+ explicit ProgressiveTimeoutReader(SocketId id, int32_t read_timeout_ms, ProgressiveReader* reader):
+ _socket_id(id),
+ _read_timeout_ms(read_timeout_ms),
+ _reader(reader),
+ _timeout_id(0),
+ _is_read_timeout(false) {
+ AddIdleReadTimeoutMonitor();
+ }
+
+ ~ProgressiveTimeoutReader() {
+ if(_timeout_id > 0) {
+ bthread_timer_del(_timeout_id);
+ }
+ }
+
+ butil::Status OnReadOnePart(const void* data, size_t length) {
+ return _reader->OnReadOnePart(data, length);
+ }
Review Comment:
This timeout reader is intended to detect *idle* progressive reads, but the timer is only armed once in the constructor. As written, it triggers after a fixed duration from the start even if data is continuously arriving. Re-arm the timer whenever a new part is read.
This issue also appears on line 199 of the same file.
##########
src/brpc/controller.cpp:
##########
@@ -174,6 +175,80 @@ class IgnoreAllRead : public ProgressiveReader {
void OnEndOfMessage(const butil::Status&) {}
};
+class ProgressiveTimeoutReader : public ProgressiveReader {
+public:
+ explicit ProgressiveTimeoutReader(SocketId id, int32_t read_timeout_ms, ProgressiveReader* reader):
+ _socket_id(id),
+ _read_timeout_ms(read_timeout_ms),
+ _reader(reader),
+ _timeout_id(0),
+ _is_read_timeout(false) {
+ AddIdleReadTimeoutMonitor();
+ }
+
+ ~ProgressiveTimeoutReader() {
+ if(_timeout_id > 0) {
+ bthread_timer_del(_timeout_id);
+ }
+ }
+
+ butil::Status OnReadOnePart(const void* data, size_t length) {
+ return _reader->OnReadOnePart(data, length);
+ }
+
+ void OnEndOfMessage(const butil::Status& status) {
+ if (_is_read_timeout) {
+ _reader->OnEndOfMessage(butil::Status(EPROGREADTIMEOUT, "The progressive read timeout"));
+ } else {
+ _reader->OnEndOfMessage(status);
+ }
+ if(_timeout_id > 0) {
+ bthread_timer_del(_timeout_id);
+ _timeout_id = 0;
+ }
+ }
+
+private:
+ static void HandleIdleProgressiveReader(void* arg) {
+ if(arg == nullptr){
+ LOG(ERROR) << "Controller::HandleIdleProgressiveReader arg is null.";
+ return;
+ }
+ ProgressiveTimeoutReader* reader = static_cast<ProgressiveTimeoutReader*>(arg);
+ SocketUniquePtr s;
+ if (Socket::Address(reader->_socket_id, &s) != 0) {
+ LOG(ERROR) << "not found the socket id : " << reader->_socket_id;
+ return;
+ }
+ auto log_idle = FLAGS_log_idle_progressive_read_close;
+ reader->_is_read_timeout = true;
+ LOG_IF(INFO, log_idle) << "progressive read timeout socket id : " << reader->_socket_id
+ << " progressive read timeout us : " << reader->_read_timeout_ms;
+ if (s->parsing_context() != NULL) {
+ s->parsing_context()->Destroy();
+ }
+ s->ReleaseReferenceIfIdle(0);
Review Comment:
Destroying s->parsing_context() from a timer thread is unsafe: http_rpc_protocol.cpp explicitly notes that calling HttpContext::Destroy() directly is wrong outside ProcessHttpXXX, and this also risks races with the socket's parsing thread. To close the connection on timeout, mark the socket failed instead of destroying the parsing context here.
##########
example/http_c++/http_server.cpp:
##########
@@ -31,6 +31,7 @@ DEFINE_int32(idle_timeout_s, -1, "Connection will be closed if there is no "
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");
Review Comment:
Spelling: "attachement" -> "attachment" in the flag description (also makes the help output more professional).
##########
src/brpc/errno.proto:
##########
@@ -41,6 +41,7 @@ enum Errno {
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
Review Comment:
EPROGREADTIMEOUT is added to errno.proto, but there is no corresponding BAIDU_REGISTER_ERRNO mapping in controller.cpp. Without registering, code paths that rely on the errno registry may show an unhelpful numeric code. Please register this errno alongside the other client-side errnos.
##########
src/brpc/controller.cpp:
##########
@@ -336,6 +412,15 @@ void Controller::Call::Reset() {
stream_user_data = NULL;
}
+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";
+ }
+}
Review Comment:
This clamp/check is ineffective because the parameter type is int32_t (it can never exceed 0x7fffffff). The warning message also mentions "seconds" even though this API is in milliseconds. Consider simplifying the setter to a straight assignment (or change the API to accept int64_t if you really need clamping).
##########
src/brpc/policy/http_rpc_protocol.h:
##########
@@ -87,11 +87,20 @@ class HttpContext : public ReadableProgressiveAttachment
, public InputMessageBase
, public HttpMessage {
public:
+ SocketId GetSocketId() override {
+ return _socket_id;
+ }
+
+ void SetSocketId(SocketId id) {
+ _socket_id = id;
+ }
+
explicit HttpContext(bool read_body_progressively,
HttpMethod request_method = HTTP_METHOD_GET)
: InputMessageBase()
, HttpMessage(read_body_progressively, request_method)
- , _is_stage2(false) {
+ , _is_stage2(false)
+ , _socket_id(0) {
// add one ref for Destroy
Review Comment:
HttpContext initializes _socket_id to 0, but the codebase defines INVALID_SOCKET_ID for the sentinel value. Using 0 can mask bugs in cases where SetSocketId() was not called (e.g., UTs), and it may result in failed lookups against Socket::Address().
##########
src/brpc/controller.cpp:
##########
@@ -174,6 +175,80 @@ class IgnoreAllRead : public ProgressiveReader {
void OnEndOfMessage(const butil::Status&) {}
};
+class ProgressiveTimeoutReader : public ProgressiveReader {
+public:
+ explicit ProgressiveTimeoutReader(SocketId id, int32_t read_timeout_ms, ProgressiveReader* reader):
+ _socket_id(id),
+ _read_timeout_ms(read_timeout_ms),
+ _reader(reader),
+ _timeout_id(0),
+ _is_read_timeout(false) {
+ AddIdleReadTimeoutMonitor();
+ }
+
+ ~ProgressiveTimeoutReader() {
+ if(_timeout_id > 0) {
+ bthread_timer_del(_timeout_id);
+ }
+ }
+
+ butil::Status OnReadOnePart(const void* data, size_t length) {
+ return _reader->OnReadOnePart(data, length);
+ }
+
+ void OnEndOfMessage(const butil::Status& status) {
+ if (_is_read_timeout) {
+ _reader->OnEndOfMessage(butil::Status(EPROGREADTIMEOUT, "The progressive read timeout"));
+ } else {
+ _reader->OnEndOfMessage(status);
+ }
+ if(_timeout_id > 0) {
+ bthread_timer_del(_timeout_id);
+ _timeout_id = 0;
+ }
+ }
+
+private:
+ static void HandleIdleProgressiveReader(void* arg) {
+ if(arg == nullptr){
+ LOG(ERROR) << "Controller::HandleIdleProgressiveReader arg is null.";
+ return;
+ }
+ ProgressiveTimeoutReader* reader = static_cast<ProgressiveTimeoutReader*>(arg);
+ SocketUniquePtr s;
+ if (Socket::Address(reader->_socket_id, &s) != 0) {
+ LOG(ERROR) << "not found the socket id : " << reader->_socket_id;
+ return;
+ }
+ auto log_idle = FLAGS_log_idle_progressive_read_close;
+ reader->_is_read_timeout = true;
+ LOG_IF(INFO, log_idle) << "progressive read timeout socket id : " << reader->_socket_id
+ << " progressive read timeout us : " << reader->_read_timeout_ms;
Review Comment:
The log message says the timeout is in "us", but _read_timeout_ms is in milliseconds and is used with milliseconds_from_now(). This makes the log misleading when diagnosing timeouts.
##########
example/http_c++/http_client.cpp:
##########
@@ -36,6 +39,25 @@ namespace brpc {
DECLARE_bool(http_verbose);
}
+class PartDataReader: public brpc::ProgressiveReader {
+public:
+ explicit PartDataReader(bthread::CountdownEvent* done): _done(done){}
+
+ butil::Status OnReadOnePart(const void* data, size_t length) {
+ memcpy(_buffer, data, length);
+ LOG(INFO) << "data ： " << _buffer << " size : " << length;
+ return butil::Status::OK();
+ }
Review Comment:
This example reader copies arbitrary-length data into a fixed 1KB buffer without bounds checking and then logs it as a C string without ensuring NUL-termination. This can overflow the buffer and/or read past the copied data when logging.
This issue also appears on line 52 of the same file.
##########
src/brpc/controller.cpp:
##########
@@ -1611,6 +1696,10 @@ void Controller::ReadProgressiveAttachmentBy(ProgressiveReader* r) {
__FUNCTION__));
}
add_flag(FLAGS_PROGRESSIVE_READER);
+ if (progressive_read_timeout_ms() > 0) {
+ auto reader = new ProgressiveTimeoutReader(_rpa->GetSocketId(), _progressive_read_timeout_ms, r);
+ return _rpa->ReadProgressiveAttachmentBy(reader);
+ }
Review Comment:
New progressive-read timeout behavior is introduced here, but there are existing progressive-read unit tests (test/brpc_http_rpc_protocol_unittest.cpp) and none cover the timeout path. Please add a unit test that verifies an idle progressive read closes the socket and the reader observes EPROGREADTIMEOUT (or the expected failure).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Progressive timeout dev (brpc)
Posted by "zchuango (via GitHub)" <gi...@apache.org>.
zchuango commented on PR #3409:
URL: https://github.com/apache/brpc/pull/3409#issuecomment-5217531297
@wwbmmm Thanks for the review. I have pushed updates addressing the remaining Copilot comments and resolved the corresponding review threads.
The idle deadline is now refreshed after each successful read, timeout cleanup goes through `Socket::SetFailed()`, and the timer and reader lifetimes are coordinated to avoid unsafe callback access. The related socket ID initialization, errno registration, header dependency, examples, and tests were also updated.
Validation:
- All six newly added focused tests passed.
- The full `brpc_http_rpc_protocol_unittest` suite passed (52/52).
- Verified in the local Docker build environment.
Could you please take another look when convenient? Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Progressive timeout dev (brpc)
Posted by "zchuango (via GitHub)" <gi...@apache.org>.
zchuango commented on code in PR #3409:
URL: https://github.com/apache/brpc/pull/3409#discussion_r3735894983
##########
src/brpc/controller.cpp:
##########
@@ -336,6 +412,15 @@ void Controller::Call::Reset() {
stream_user_data = NULL;
}
+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";
+ }
+}
Review Comment:
Fixed. The ineffective clamp was removed, and the int32_t timeout value is now assigned directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Progressive timeout dev (brpc)
Posted by "zchuango (via GitHub)" <gi...@apache.org>.
zchuango commented on code in PR #3409:
URL: https://github.com/apache/brpc/pull/3409#discussion_r3735926148
##########
src/brpc/errno.proto:
##########
@@ -41,6 +41,7 @@ enum Errno {
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
Review Comment:
Fixed. EPROGREADTIMEOUT is now registered with BAIDU_REGISTER_ERRNO in controller.cpp, alongside the other client-side error codes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Progressive timeout dev (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on code in PR #3409:
URL: https://github.com/apache/brpc/pull/3409#discussion_r3679777949
##########
src/brpc/controller.h:
##########
@@ -48,7 +48,6 @@
#include "brpc/grpc.h"
#include "brpc/kvmap.h"
#include "brpc/rpc_dump.h"
-
Review Comment:
don't remove this line
##########
src/brpc/controller.cpp:
##########
@@ -174,6 +175,80 @@ class IgnoreAllRead : public ProgressiveReader {
void OnEndOfMessage(const butil::Status&) {}
};
+class ProgressiveTimeoutReader : public ProgressiveReader {
+public:
+ explicit ProgressiveTimeoutReader(SocketId id, int32_t read_timeout_ms, ProgressiveReader* reader):
+ _socket_id(id),
+ _read_timeout_ms(read_timeout_ms),
+ _reader(reader),
+ _timeout_id(0),
+ _is_read_timeout(false) {
+ AddIdleReadTimeoutMonitor();
+ }
+
+ ~ProgressiveTimeoutReader() {
+ if(_timeout_id > 0) {
+ bthread_timer_del(_timeout_id);
+ }
+ }
+
+ butil::Status OnReadOnePart(const void* data, size_t length) {
+ return _reader->OnReadOnePart(data, length);
+ }
+
+ void OnEndOfMessage(const butil::Status& status) {
+ if (_is_read_timeout) {
+ _reader->OnEndOfMessage(butil::Status(EPROGREADTIMEOUT, "The progressive read timeout"));
+ } else {
+ _reader->OnEndOfMessage(status);
+ }
+ if(_timeout_id > 0) {
+ bthread_timer_del(_timeout_id);
+ _timeout_id = 0;
+ }
+ }
+
+private:
+ static void HandleIdleProgressiveReader(void* arg) {
+ if(arg == nullptr){
+ LOG(ERROR) << "Controller::HandleIdleProgressiveReader arg is null.";
+ return;
+ }
+ ProgressiveTimeoutReader* reader = static_cast<ProgressiveTimeoutReader*>(arg);
+ SocketUniquePtr s;
+ if (Socket::Address(reader->_socket_id, &s) != 0) {
+ LOG(ERROR) << "not found the socket id : " << reader->_socket_id;
+ return;
+ }
+ auto log_idle = FLAGS_log_idle_progressive_read_close;
+ reader->_is_read_timeout = true;
+ LOG_IF(INFO, log_idle) << "progressive read timeout socket id : " << reader->_socket_id
+ << " progressive read timeout us : " << reader->_read_timeout_ms;
+ if (s->parsing_context() != NULL) {
+ s->parsing_context()->Destroy();
+ }
+ s->ReleaseReferenceIfIdle(0);
+ }
Review Comment:
add a new line
##########
src/brpc/controller.h:
##########
@@ -179,7 +178,6 @@ friend void policy::ProcessThriftRequest(InputMessageBase*);
uint64_t log_id;
std::string request_id;
};
-
Review Comment:
don't remove this line
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Progressive timeout dev (brpc)
Posted by "zchuango (via GitHub)" <gi...@apache.org>.
zchuango commented on code in PR #3409:
URL: https://github.com/apache/brpc/pull/3409#discussion_r3735812573
##########
src/brpc/controller.cpp:
##########
@@ -174,6 +175,80 @@ class IgnoreAllRead : public ProgressiveReader {
void OnEndOfMessage(const butil::Status&) {}
};
+class ProgressiveTimeoutReader : public ProgressiveReader {
+public:
+ explicit ProgressiveTimeoutReader(SocketId id, int32_t read_timeout_ms, ProgressiveReader* reader):
+ _socket_id(id),
+ _read_timeout_ms(read_timeout_ms),
+ _reader(reader),
+ _timeout_id(0),
+ _is_read_timeout(false) {
+ AddIdleReadTimeoutMonitor();
+ }
+
+ ~ProgressiveTimeoutReader() {
+ if(_timeout_id > 0) {
+ bthread_timer_del(_timeout_id);
+ }
+ }
+
+ butil::Status OnReadOnePart(const void* data, size_t length) {
+ return _reader->OnReadOnePart(data, length);
+ }
+
+ void OnEndOfMessage(const butil::Status& status) {
+ if (_is_read_timeout) {
+ _reader->OnEndOfMessage(butil::Status(EPROGREADTIMEOUT, "The progressive read timeout"));
+ } else {
+ _reader->OnEndOfMessage(status);
+ }
+ if(_timeout_id > 0) {
+ bthread_timer_del(_timeout_id);
+ _timeout_id = 0;
+ }
+ }
+
+private:
+ static void HandleIdleProgressiveReader(void* arg) {
+ if(arg == nullptr){
+ LOG(ERROR) << "Controller::HandleIdleProgressiveReader arg is null.";
+ return;
+ }
+ ProgressiveTimeoutReader* reader = static_cast<ProgressiveTimeoutReader*>(arg);
+ SocketUniquePtr s;
+ if (Socket::Address(reader->_socket_id, &s) != 0) {
+ LOG(ERROR) << "not found the socket id : " << reader->_socket_id;
+ return;
+ }
+ auto log_idle = FLAGS_log_idle_progressive_read_close;
+ reader->_is_read_timeout = true;
+ LOG_IF(INFO, log_idle) << "progressive read timeout socket id : " << reader->_socket_id
+ << " progressive read timeout us : " << reader->_read_timeout_ms;
Review Comment:
Fixed. HttpContext::_socket_id is now initialized to INVALID_SOCKET_ID, so an unset socket ID cannot be mistaken for a valid one.
##########
src/brpc/controller.cpp:
##########
@@ -174,6 +175,80 @@ class IgnoreAllRead : public ProgressiveReader {
void OnEndOfMessage(const butil::Status&) {}
};
+class ProgressiveTimeoutReader : public ProgressiveReader {
+public:
+ explicit ProgressiveTimeoutReader(SocketId id, int32_t read_timeout_ms, ProgressiveReader* reader):
+ _socket_id(id),
+ _read_timeout_ms(read_timeout_ms),
+ _reader(reader),
+ _timeout_id(0),
+ _is_read_timeout(false) {
+ AddIdleReadTimeoutMonitor();
+ }
+
+ ~ProgressiveTimeoutReader() {
+ if(_timeout_id > 0) {
+ bthread_timer_del(_timeout_id);
+ }
+ }
+
+ butil::Status OnReadOnePart(const void* data, size_t length) {
+ return _reader->OnReadOnePart(data, length);
+ }
+
+ void OnEndOfMessage(const butil::Status& status) {
+ if (_is_read_timeout) {
+ _reader->OnEndOfMessage(butil::Status(EPROGREADTIMEOUT, "The progressive read timeout"));
+ } else {
+ _reader->OnEndOfMessage(status);
+ }
+ if(_timeout_id > 0) {
+ bthread_timer_del(_timeout_id);
+ _timeout_id = 0;
+ }
+ }
+
+private:
+ static void HandleIdleProgressiveReader(void* arg) {
+ if(arg == nullptr){
+ LOG(ERROR) << "Controller::HandleIdleProgressiveReader arg is null.";
+ return;
+ }
+ ProgressiveTimeoutReader* reader = static_cast<ProgressiveTimeoutReader*>(arg);
+ SocketUniquePtr s;
+ if (Socket::Address(reader->_socket_id, &s) != 0) {
+ LOG(ERROR) << "not found the socket id : " << reader->_socket_id;
+ return;
+ }
+ auto log_idle = FLAGS_log_idle_progressive_read_close;
+ reader->_is_read_timeout = true;
+ LOG_IF(INFO, log_idle) << "progressive read timeout socket id : " << reader->_socket_id
+ << " progressive read timeout us : " << reader->_read_timeout_ms;
Review Comment:
Fixed. HttpContext::_socket_id is now initialized to INVALID_SOCKET_ID, so an unset socket ID cannot be mistaken for a valid one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Progressive timeout dev (brpc)
Posted by "zchuango (via GitHub)" <gi...@apache.org>.
zchuango commented on PR #3409:
URL: https://github.com/apache/brpc/pull/3409#issuecomment-5310893875
> @zchuango After this PR merge, the CI become unstable. The test case always fail.`HttpTest_progressive_read_timeout_preserves_reader_error`
>
> See these logs for details: https://github.com/apache/brpc/actions/runs/31922283751/job/95104007486?pr=3450 https://github.com/apache/brpc/actions/runs/31922283751/job/95104007467?pr=3450
>
> Could you take a look?
Thanks for reporting this. I will investigate the failing test and look into whether there is any issue with the interaction between the progressive read timeout callback and reader error handling.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Progressive timeout dev (brpc)
Posted by "zchuango (via GitHub)" <gi...@apache.org>.
zchuango commented on code in PR #3409:
URL: https://github.com/apache/brpc/pull/3409#discussion_r3735810747
##########
src/brpc/progressive_reader.h:
##########
@@ -20,6 +20,7 @@
#define BRPC_PROGRESSIVE_READER_H
#include "brpc/shared_object.h"
+#include "brpc/socket.h"
Review Comment:
Fixed. brpc/socket.h has been replaced with brpc/socket_id.h, since this interface only requires SocketId.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Progressive timeout dev (brpc)
Posted by "zchuango (via GitHub)" <gi...@apache.org>.
zchuango commented on code in PR #3409:
URL: https://github.com/apache/brpc/pull/3409#discussion_r3735891785
##########
src/brpc/controller.cpp:
##########
@@ -174,6 +175,80 @@ class IgnoreAllRead : public ProgressiveReader {
void OnEndOfMessage(const butil::Status&) {}
};
+class ProgressiveTimeoutReader : public ProgressiveReader {
+public:
+ explicit ProgressiveTimeoutReader(SocketId id, int32_t read_timeout_ms, ProgressiveReader* reader):
+ _socket_id(id),
+ _read_timeout_ms(read_timeout_ms),
+ _reader(reader),
+ _timeout_id(0),
+ _is_read_timeout(false) {
+ AddIdleReadTimeoutMonitor();
+ }
+
+ ~ProgressiveTimeoutReader() {
+ if(_timeout_id > 0) {
+ bthread_timer_del(_timeout_id);
+ }
+ }
+
+ butil::Status OnReadOnePart(const void* data, size_t length) {
+ return _reader->OnReadOnePart(data, length);
+ }
+
+ void OnEndOfMessage(const butil::Status& status) {
+ if (_is_read_timeout) {
+ _reader->OnEndOfMessage(butil::Status(EPROGREADTIMEOUT, "The progressive read timeout"));
+ } else {
+ _reader->OnEndOfMessage(status);
+ }
+ if(_timeout_id > 0) {
+ bthread_timer_del(_timeout_id);
+ _timeout_id = 0;
+ }
+ }
+
+private:
+ static void HandleIdleProgressiveReader(void* arg) {
+ if(arg == nullptr){
+ LOG(ERROR) << "Controller::HandleIdleProgressiveReader arg is null.";
+ return;
+ }
+ ProgressiveTimeoutReader* reader = static_cast<ProgressiveTimeoutReader*>(arg);
+ SocketUniquePtr s;
+ if (Socket::Address(reader->_socket_id, &s) != 0) {
+ LOG(ERROR) << "not found the socket id : " << reader->_socket_id;
+ return;
+ }
+ auto log_idle = FLAGS_log_idle_progressive_read_close;
+ reader->_is_read_timeout = true;
+ LOG_IF(INFO, log_idle) << "progressive read timeout socket id : " << reader->_socket_id
+ << " progressive read timeout us : " << reader->_read_timeout_ms;
+ if (s->parsing_context() != NULL) {
+ s->parsing_context()->Destroy();
+ }
+ s->ReleaseReferenceIfIdle(0);
Review Comment:
Fixed. The timer callback no longer destroys the parsing context directly. It obtains a referenced socket through Socket::Address() and calls Socket::SetFailed(), allowing the normal socket failure path to perform cleanup safely.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Progressive timeout dev (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm merged PR #3409:
URL: https://github.com/apache/brpc/pull/3409
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Progressive timeout dev (brpc)
Posted by "zchuango (via GitHub)" <gi...@apache.org>.
zchuango commented on PR #3409:
URL: https://github.com/apache/brpc/pull/3409#issuecomment-5144263939
Thanks for your review. I have noticed this problem and will address it as soon as possible. Appreciate your help! @wwbmmm
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Progressive timeout dev (brpc)
Posted by "zchuango (via GitHub)" <gi...@apache.org>.
zchuango commented on code in PR #3409:
URL: https://github.com/apache/brpc/pull/3409#discussion_r3735915264
##########
src/brpc/controller.cpp:
##########
@@ -1611,6 +1696,10 @@ void Controller::ReadProgressiveAttachmentBy(ProgressiveReader* r) {
__FUNCTION__));
}
add_flag(FLAGS_PROGRESSIVE_READER);
+ if (progressive_read_timeout_ms() > 0) {
+ auto reader = new ProgressiveTimeoutReader(_rpa->GetSocketId(), _progressive_read_timeout_ms, r);
+ return _rpa->ReadProgressiveAttachmentBy(reader);
+ }
Review Comment:
Fixed. Added unit tests covering active reads, idle timeout after receiving data, timeout before the first body part, slow user callbacks, reader error propagation, and unsupported HTTP/2 contexts.
Validation:
All six newly added focused tests passed.
The full brpc_http_rpc_protocol_unittest suite passed (52/52).
Verified in the local Docker build environment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Progressive timeout dev (brpc)
Posted by "zchuango (via GitHub)" <gi...@apache.org>.
zchuango commented on code in PR #3409:
URL: https://github.com/apache/brpc/pull/3409#discussion_r3735929883
##########
example/http_c++/http_client.cpp:
##########
@@ -36,6 +39,25 @@ namespace brpc {
DECLARE_bool(http_verbose);
}
+class PartDataReader: public brpc::ProgressiveReader {
+public:
+ explicit PartDataReader(bthread::CountdownEvent* done): _done(done){}
+
+ butil::Status OnReadOnePart(const void* data, size_t length) {
+ memcpy(_buffer, data, length);
+ LOG(INFO) << "data ： " << _buffer << " size : " << length;
+ return butil::Status::OK();
+ }
Review Comment:
Fixed. The example now constructs a length-aware std::string directly from each data part, avoiding the fixed-size buffer overflow and NUL-termination issues.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Progressive timeout dev (brpc)
Posted by "zchuango (via GitHub)" <gi...@apache.org>.
zchuango commented on code in PR #3409:
URL: https://github.com/apache/brpc/pull/3409#discussion_r3735884848
##########
src/brpc/controller.cpp:
##########
@@ -174,6 +175,80 @@ class IgnoreAllRead : public ProgressiveReader {
void OnEndOfMessage(const butil::Status&) {}
};
+class ProgressiveTimeoutReader : public ProgressiveReader {
+public:
+ explicit ProgressiveTimeoutReader(SocketId id, int32_t read_timeout_ms, ProgressiveReader* reader):
+ _socket_id(id),
+ _read_timeout_ms(read_timeout_ms),
+ _reader(reader),
+ _timeout_id(0),
+ _is_read_timeout(false) {
+ AddIdleReadTimeoutMonitor();
+ }
+
+ ~ProgressiveTimeoutReader() {
+ if(_timeout_id > 0) {
+ bthread_timer_del(_timeout_id);
+ }
+ }
+
+ butil::Status OnReadOnePart(const void* data, size_t length) {
+ return _reader->OnReadOnePart(data, length);
+ }
+
+ void OnEndOfMessage(const butil::Status& status) {
+ if (_is_read_timeout) {
+ _reader->OnEndOfMessage(butil::Status(EPROGREADTIMEOUT, "The progressive read timeout"));
+ } else {
+ _reader->OnEndOfMessage(status);
+ }
+ if(_timeout_id > 0) {
+ bthread_timer_del(_timeout_id);
+ _timeout_id = 0;
+ }
+ }
+
+private:
+ static void HandleIdleProgressiveReader(void* arg) {
+ if(arg == nullptr){
+ LOG(ERROR) << "Controller::HandleIdleProgressiveReader arg is null.";
+ return;
+ }
+ ProgressiveTimeoutReader* reader = static_cast<ProgressiveTimeoutReader*>(arg);
+ SocketUniquePtr s;
+ if (Socket::Address(reader->_socket_id, &s) != 0) {
+ LOG(ERROR) << "not found the socket id : " << reader->_socket_id;
+ return;
+ }
+ auto log_idle = FLAGS_log_idle_progressive_read_close;
+ reader->_is_read_timeout = true;
+ LOG_IF(INFO, log_idle) << "progressive read timeout socket id : " << reader->_socket_id
+ << " progressive read timeout us : " << reader->_read_timeout_ms;
Review Comment:
Fixed. The timeout message now reports the duration in milliseconds, matching progressive_read_timeout_ms.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Progressive timeout dev (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on PR #3409:
URL: https://github.com/apache/brpc/pull/3409#issuecomment-5150416443
Please resolve Copilot's comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Progressive timeout dev (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on PR #3409:
URL: https://github.com/apache/brpc/pull/3409#issuecomment-5306298597
@zchuango After this PR merge, the CI become unstable. The test case `HttpTest_progressive_read_timeout_preserves_reader_error` always fail.
See these logs for details:
https://github.com/apache/brpc/actions/runs/31922283751/job/95104007486?pr=3450
https://github.com/apache/brpc/actions/runs/31922283751/job/95104007467?pr=3450
Could you take a look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org