Copilot commented on code in PR #3621:
URL: https://github.com/apache/brpc/pull/3621#discussion_r4355501988
##########
src/brpc/socket.h:
##########
@@ -705,7 +705,7 @@ friend class TransportFactory;
}
// Returns true if the remote side is overcrowded.
Review Comment:
This comment appears inaccurate: `is_overcrowded()` is derived from local
`_unwritten_bytes` (pending output), not the remote side. Please adjust the
comment to reflect local pending-write/backpressure semantics to avoid
misleading API consumers.
##########
test/brpc_http_rpc_protocol_unittest.cpp:
##########
@@ -2319,6 +2320,43 @@ 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_tracks_pending_writes) {
+ int fds[2];
+ ASSERT_EQ(0, pipe(fds));
+ butil::fd_guard read_fd(fds[0]);
+ ASSERT_EQ(0, butil::make_non_blocking(read_fd));
+ brpc::SocketOptions options;
+ options.fd = fds[1];
+ brpc::SocketId id;
+ ASSERT_EQ(0, brpc::Socket::Create(options, &id));
Review Comment:
This test doesn’t guard `fds[1]` against early returns. If `Socket::Create`
fails, the write-end FD will leak. Consider wrapping `fds[1]` in a
`butil::fd_guard` and releasing it only after ownership is successfully
transferred to the socket.
##########
test/brpc_http_rpc_protocol_unittest.cpp:
##########
@@ -2319,6 +2320,43 @@ 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_tracks_pending_writes) {
+ int fds[2];
+ ASSERT_EQ(0, pipe(fds));
+ butil::fd_guard read_fd(fds[0]);
+ ASSERT_EQ(0, butil::make_non_blocking(read_fd));
+ brpc::SocketOptions options;
+ options.fd = fds[1];
+ brpc::SocketId id;
+ ASSERT_EQ(0, brpc::Socket::Create(options, &id));
+ brpc::SocketUniquePtr sock;
+ ASSERT_EQ(0, brpc::Socket::Address(id, &sock));
+
+ GFLAGS_NAMESPACE::FlagSaver flag_saver;
+ brpc::FLAGS_socket_max_unwritten_bytes = 64;
+
+ butil::IOBuf data;
+ data.append(std::string(1024 * 1024, 'x'));
+ ASSERT_EQ(0, sock->Write(&data));
+ ASSERT_TRUE(sock->is_overcrowded());
+
+ brpc::FLAGS_socket_max_unwritten_bytes = 0;
+ ASSERT_FALSE(sock->is_overcrowded());
+ brpc::FLAGS_socket_max_unwritten_bytes = 64;
+
+ char buf[8192];
+ const int max_wait_iterations = 1000;
+ for (int i = 0;
+ i < max_wait_iterations && sock->is_overcrowded(); ++i) {
+ ssize_t nread;
+ while ((nread = read(read_fd, buf, sizeof(buf))) > 0) {}
+ ASSERT_EQ(-1, nread);
+ ASSERT_TRUE(errno == EAGAIN || errno == EWOULDBLOCK);
+ bthread_usleep(1000);
+ }
Review Comment:
The test mutates `FLAGS_socket_max_unwritten_bytes` while the socket may
have background activity (e.g., KeepWrite) reading the flag, which can create a
TSAN-visible race because gflags assignments are not guaranteed to be
synchronized. To keep the test race-free, avoid changing the flag while the
socket is active (e.g., set the flag before creating/using the socket, or
close/quiesce the socket before toggling), or otherwise ensure the flag changes
are serialized with the socket activity.
##########
src/brpc/socket.cpp:
##########
@@ -89,8 +89,8 @@ 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.");
Review Comment:
The PR description says a non-positive `socket_max_unwritten_bytes` disables
overcrowding, but the flag help text doesn’t mention this. Update the
description string to document that `<= 0` disables the overcrowding check so
operators don’t misconfigure/assume `0` is a strict limit.
--
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]
