You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by "chenBright (via GitHub)" <gi...@apache.org> on 2025/06/28 08:50:04 UTC
[PR] Bugfix: Socket without health check was abnormally recycled (brpc)
chenBright opened a new pull request, #3010:
URL: https://github.com/apache/brpc/pull/3010
### What problem does this PR solve?
Issue Number: resolve #2998
Problem Summary:
When the health check is turned off, the SocketMap does not hold a reference to the Socket, so the Socket will be abnormally recycled when the connection is closed.
### What is changed and the side effects?
Changed:
Regardless of whether the health check is enabled, SocketMap must hold a reference to the socket.
Side effects:
- Performance effects:
- Breaking backward compatibility:
---
### 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] Bugfix: Socket without health check was abnormally recycled (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3010:
URL: https://github.com/apache/brpc/pull/3010#discussion_r2173183494
##########
src/brpc/selective_channel.cpp:
##########
@@ -197,10 +197,14 @@ int ChannelBalancer::AddChannel(ChannelBase* sub_channel,
}
SocketUniquePtr ptr;
int rc = Socket::AddressFailedAsWell(sock_id, &ptr);
- if (rc < 0 || (rc > 0 && !ptr->HCEnabled())) {
+ if (rc < 0) {
LOG(FATAL) << "Fail to address SocketId=" << sock_id;
return -1;
}
+ if (rc > 0 && !ptr->HCEnabled()) {
+ LOG(FATAL) << "Health check of SocketId=" << sock_id << " is disable";
Review Comment:
The error message is grammatically incorrect. Change "is disable" to "is disabled".
```suggestion
LOG(FATAL) << "Health check of SocketId=" << sock_id << " is disabled";
```
##########
test/brpc_socket_map_unittest.cpp:
##########
@@ -140,6 +141,32 @@ TEST_F(SocketMapTest, max_pool_size) {
EXPECT_TRUE(ptrs[i]->Failed());
}
}
+
+TEST_F(SocketMapTest, disable_health_check) {
+ int32_t old_interval = brpc::FLAGS_health_check_interval;
+ brpc::FLAGS_health_check_interval = 0;
+ brpc::SocketId id;
+ ASSERT_EQ(-1, brpc::SocketMapFind(g_key, &id));
+ ASSERT_EQ(0, brpc::SocketMapInsert(g_key, &id));
+ ASSERT_EQ(0, brpc::SocketMapFind(g_key, &id));
+ {
+ brpc::SocketUniquePtr ptr;
+ ASSERT_EQ(0, brpc::Socket::Address(id, &ptr));
+ }
+ ASSERT_EQ(0, brpc::Socket::SetFailed(id));
+
+ brpc::SocketUniquePtr ptr;
+ // The socket should not be recycled,
+ // because SocketMap holds a reference to it.
+ ASSERT_EQ(1, brpc::Socket::AddressFailedAsWell(id, &ptr));
+ ASSERT_EQ(2, ptr->nref());
+ brpc::SocketMapRemove(g_key);
+ // After removing the socket, `ptr' holds the last reference.
+ ASSERT_EQ(1, ptr->nref());
+ ASSERT_EQ(-1, brpc::SocketMapFind(g_key, &id));
+ brpc::FLAGS_health_check_interval = old_interval;
Review Comment:
Restoring the flag only at the end of the test can leave side effects if an assertion fails earlier. Consider using an RAII or scoped helper to reset the flag even on failures.
```suggestion
```
--
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] Bugfix: Socket without health check was abnormally recycled (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm merged PR #3010:
URL: https://github.com/apache/brpc/pull/3010
--
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