Copilot commented on code in PR #3424:
URL: https://github.com/apache/brpc/pull/3424#discussion_r3700996920
##########
src/brpc/rdma/rdma_endpoint.cpp:
##########
@@ -1097,10 +1107,10 @@ int RdmaEndpoint::AllocateResources() {
}
if (!FLAGS_rdma_use_polling) {
- if (0 != ReqNotifyCq(true)) {
+ if (0 != ReqNotifyCq(true, false)) {
return -1;
}
- if (0 != ReqNotifyCq(false)) {
+ if (0 != ReqNotifyCq(false, false)) {
return -1;
Review Comment:
`AllocateResources()` failures are treated as recoverable (handshake falls
back to TCP and keeps the socket alive), but if `ReqNotifyCq(...)` fails here
`_resource` has already been allocated and is left attached to the endpoint.
That can leak RDMA resources/FDs for the lifetime of an otherwise-healthy TCP
connection. Clean up `_resource` before returning so the fallback path doesn't
retain partially initialized RDMA state.
##########
test/brpc_rdma_unittest.cpp:
##########
@@ -1919,6 +1920,108 @@ TEST_F(RdmaTest,
v3_server_reply_has_no_ece_without_hw_negotiation) {
StopServer();
}
+struct ResourceAllocFailGuard {
+ ResourceAllocFailGuard() {
+ rdma::g_fail_resource_alloc_for_test = true;
+ }
+ ~ResourceAllocFailGuard() {
+ rdma::g_fail_resource_alloc_for_test = false;
+ }
+};
Review Comment:
`ResourceAllocFailGuard` always resets the global flag to `false` on scope
exit instead of restoring the previous value. This can make the guard unsafe to
reuse (e.g., nested guards or tests that intentionally set the flag before
using the guard). Preserve and restore the prior value like other RAII flag
guards in this file.
##########
src/brpc/rdma/rdma_endpoint.h:
##########
@@ -244,7 +244,7 @@ friend int v3_wire::WriteV3Hello(RdmaEndpoint*, const
RdmaHello&);
int GetAndAckEvents(SocketUniquePtr& s);
// Request completion notification on a send/recv CQ.
- int ReqNotifyCq(bool send_cq);
+ int ReqNotifyCq(bool send_cq, bool fatal_on_error);
Review Comment:
The new `fatal_on_error` parameter changes the semantics of `ReqNotifyCq`
(handshake can treat failures as recoverable while the established-RDMA path
treats them as fatal). Updating the comment to document this distinction will
help avoid incorrect call sites in the future.
--
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]
