wwbmmm commented on code in PR #3621:
URL: https://github.com/apache/brpc/pull/3621#discussion_r4348839219
##########
src/brpc/socket.cpp:
##########
@@ -3003,11 +2999,14 @@ void Socket::AddInputMessages(size_t count) {
GetOrNewSharedPart()->in_num_messages.fetch_add(count,
butil::memory_order_relaxed);
}
void Socket::CancelUnwrittenBytes(size_t bytes) {
- const int64_t before_minus =
- _unwritten_bytes.fetch_sub(bytes, butil::memory_order_relaxed);
- if (before_minus < (int64_t)bytes + FLAGS_socket_max_unwritten_bytes) {
- _overcrowded = false;
- }
+ _unwritten_bytes.fetch_sub(bytes, butil::memory_order_relaxed);
+}
+
+bool Socket::is_overcrowded() const {
+ const int64_t unwritten_bytes =
+ _unwritten_bytes.load(butil::memory_order_relaxed);
+ return FLAGS_socket_max_unwritten_bytes > 0 &&
Review Comment:
[replied by brpc-oncall robot] Please read the flag once, e.g. `const
int64_t limit = FLAGS_socket_max_unwritten_bytes;` and use `limit` for both the
`> 0` check and the comparison. Two separate reads can observe different values
when the flag is changed at runtime, and it also saves a second load of the
global on the write path.
##########
src/brpc/socket.cpp:
##########
@@ -89,8 +89,9 @@ DEFINE_int32(ssl_handshake_timeout_ms, 5000,
BRPC_VALIDATE_GFLAG(ssl_handshake_timeout_ms, PassValidate);
DEFINE_int64(socket_max_unwritten_bytes, 64 * 1024 * 1024,
- "Max unwritten bytes in each socket, if the limit is reached,"
- " Socket.Write fails with EOVERCROWDED");
+ "Max unwritten bytes in each socket. Socket.Write fails with "
+ "EOVERCROWDED when the limit is reached; non-positive disables "
Review Comment:
[replied by brpc-oncall robot] `non-positive disables the limit` is only
true for the check in `is_overcrowded()`; other consumers of this flag don't
implement it. `HttpMessage::Append` (src/brpc/details/http_message.cpp:347)
uses `_body.size() <= FLAGS_socket_max_unwritten_bytes` to decide whether to
keep buffering, so with 0 it stops buffering and spins in the 10ms sleep loop;
`ProgressiveAttachment::Write` (src/brpc/progressive_attachment.cpp:131,164)
compares `_saved_buf.size() >= (size_t)FLAGS_socket_max_unwritten_bytes`, so 0
makes it return EOVERCROWDED for every chunk. Suggest narrowing the wording
here (e.g. "non-positive disables the EOVERCROWDED throttle of Socket.Write"),
or making the other sites consistent in a follow-up rather than documenting a
guarantee that doesn't hold.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
