wwbmmm commented on code in PR #3621:
URL: https://github.com/apache/brpc/pull/3621#discussion_r4322661875
##########
src/brpc/socket.cpp:
##########
@@ -3003,11 +2997,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 unwritten_bytes > 0 &&
Review Comment:
[replied by brpc-oncall robot] `unwritten_bytes > 0` is redundant for every
positive value of the flag (`unwritten >= max > 0` already implies `> 0`), so
the only configuration it changes is a non-positive
`-socket_max_unwritten_bytes`: the old code became permanently overcrowded
after the first write, the new code does not. If non-positive is intended to
mean "no limit", please state that explicitly
(`FLAGS_socket_max_unwritten_bytes > 0 && unwritten_bytes >=
FLAGS_socket_max_unwritten_bytes`) and document it on the flag; otherwise drop
the conjunct so this change stays a pure race fix without hidden behavior
changes.
##########
test/brpc_http_rpc_protocol_unittest.cpp:
##########
@@ -2319,6 +2319,23 @@ TEST_F(HttpTest, h2_ping_ack_respects_socket_write_cap) {
ASSERT_EQ(brpc::PARSE_ERROR_ABSOLUTELY_WRONG, pr.error());
}
+TEST_F(HttpTest, socket_overcrowded_state_follows_unwritten_bytes) {
+ GFLAGS_NAMESPACE::FlagSaver flag_saver;
+ brpc::FLAGS_socket_max_unwritten_bytes = 64;
+
+ _socket->_unwritten_bytes.store(0, butil::memory_order_relaxed);
Review Comment:
[replied by brpc-oncall robot] This test pokes the private
`_unwritten_bytes` directly, so it only exercises the new accessor and would
pass with or without the actual fix (it cannot catch the stale cached-flag
problem this PR addresses). Suggest driving the public path instead, e.g.
`Write()` a buffer larger than the temporary limit and assert
`is_overcrowded()` either side of the write/complete transition, so the test
fails on the old implementation.
--
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]
