Copilot commented on code in PR #3480:
URL: https://github.com/apache/brpc/pull/3480#discussion_r3838923745
##########
src/brpc/rdma_transport.cpp:
##########
@@ -70,6 +70,7 @@ int RdmaTransport::Reset(int32_t expected_nref) {
if (_rdma_ep) {
_rdma_ep->Reset();
_rdma_state = RDMA_UNKNOWN;
+ _on_edge_trigger = InputMessenger::OnNewMessages;
Review Comment:
RdmaTransport::Reset() unconditionally switches the edge-trigger callback to
InputMessenger::OnNewMessages. That differs from Init(), where client-side
sockets explicitly require RdmaEndpoint::OnNewDataFromTcp to drive the blocking
client handshake (wake _read_butex) and to avoid InputMessenger reading/parsing
RDMA handshake bytes. After a health-check reset, a client socket would likely
mis-handle the next handshake (or race on TCP fd reads) because it’s now using
OnNewMessages.
##########
src/brpc/rdma/rdma_endpoint.cpp:
##########
@@ -642,7 +639,13 @@ ParseResult
RdmaEndpoint::ExecuteServerHandshake(butil::IOBuf* source, Socket* s
rdma_transport->_rdma_state = RdmaTransport::RDMA_ON;
ep->_state.store(ESTABLISHED, butil::memory_order_relaxed);
s->reset_parsing_context(nullptr);
- return MakeParseError(PARSE_ERROR_TRY_OTHERS);
+ rdma_transport->_on_edge_trigger = rdma::RdmaEndpoint::OnNewDataFromTcp;
+ // Return NOT_ENOUGH_DATA (not TRY_OTHERS) so that OnNewMessages stops
+ // processing _read_buf immediately. Any remaining data in _read_buf
+ // after the 4B ACK is not valid RDMA traffic and will be overwritten by
+ // PollCq/HandleCompletion which now safely writes to _read_buf since
+ // the state is ESTABLISHED.
Review Comment:
The comment says any remaining TCP data in _read_buf after the 4B ACK “will
be overwritten” by PollCq, but HandleCompletion appends into _socket->_read_buf
and PollCq then calls InputMessenger::ProcessNewMessage on that buffer. If any
extra TCP bytes exist here, they’ll become a prefix in the RDMA parse stream
and can break parsing. It’s safer to explicitly drop/clear any unexpected
remaining bytes before transitioning to ESTABLISHED, and keep the
NOT_ENOUGH_DATA return to stop OnNewMessages from further parsing.
This issue also appears in the following locations of the same file:
- line 926
- line 932
--
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]
