Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate handshake, transport-selection, fallback, initialization, and I/O issues remain.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a shared transport-level handshake framework for RDMA and UBSHM, centralizing framing, negotiation, upgrade selection, and TCP fallback.
Changes:
- Adds common transport, session, framing, I/O, and adapter abstractions.
- Migrates RDMA and UBSHM handshake orchestration.
- Adds compatibility, fallback, handshake tests, and build integration.
File summaries
| File | Reviewed change / final finding |
|---|---|
test/brpc_ubring_unittest.cpp |
UBSHM compatibility and fallback tests. |
test/brpc_transport_handshake_unittest.cpp |
Common framing and handshake state-transition tests. |
src/brpc/ubshm/ub_endpoint.h |
UBSHM endpoint interface migration. |
src/brpc/ubshm_transport.h |
UBSHM transport declarations. |
src/brpc/ubshm_transport.cpp |
UBSHM resource and fallback integration. Moderate (3 votes): fallback leaves resources active. Moderate (1 vote): allocation failure leaves a null endpoint on an upgrade-capable transport. |
src/brpc/transport_handshake.h |
Shared handshake phases and session contracts. Critical (1 vote): relaxed phase publication can leave handshake reads blocked; release synchronization is needed. |
src/brpc/transport_handshake.cpp |
Common handshake state transitions. |
src/brpc/transport_factory.h |
Transport factory interface. |
src/brpc/transport_factory.cpp |
Adapter transport construction. |
src/brpc/socket.h |
Socket transport integration. |
src/brpc/rdma/rdma_handshake.h |
RDMA handshake declarations. |
src/brpc/rdma/rdma_handshake.cpp |
RDMA handshake implementation integration. |
src/brpc/rdma/rdma_handshake_server.h |
RDMA server handshake declarations. |
src/brpc/rdma/rdma_handshake_server.cpp |
RDMA server handshake implementation. |
src/brpc/rdma/rdma_endpoint.h |
RDMA endpoint interface. |
src/brpc/rdma_transport.h |
RDMA transport declarations. |
src/brpc/rdma_transport.cpp |
RDMA resource and fallback integration. Moderate (3 votes): fallback does not release allocated resources. Moderate (1 vote): allocation failure leaves a null endpoint on an upgrade-capable transport. |
src/brpc/rdma_handshake.proto |
RDMA handshake wire definitions. |
src/brpc/policy/transport_handshake_protocol.h |
Common handshake protocol declarations. |
src/brpc/policy/transport_handshake_protocol.cpp |
Common handshake parser dispatch. |
src/brpc/policy/rdma_handshake_protocol.h |
RDMA compatibility API declarations. |
src/brpc/policy/rdma_handshake_protocol.cpp |
RDMA compatibility facade. |
src/brpc/input_messenger.h |
Handshake and input-messenger integration. |
src/brpc/handshake/ubshm_handshake.h |
UBSHM handshake adapter declarations. |
src/brpc/handshake/ubshm_handshake.cpp |
Critical (2 votes): upgrade selection permits wrong-mode transport casts. Moderate (1 vote): coalesced application bytes are rejected. Critical (1 vote): short UBSHM names can cause a 48-byte over-read. |
src/brpc/handshake/rdma_handshake.h |
RDMA handshake adapter declarations. |
src/brpc/handshake/rdma_handshake.cpp |
Critical (2 votes): upgrade selection permits wrong-mode transport casts. Moderate (1 vote): coalesced application bytes are rejected. |
src/brpc/handshake/rdma_handshake_constants.h |
RDMA handshake wire-format constants. |
src/brpc/handshake/handshake_io.h |
Handshake I/O interface. |
src/brpc/handshake/handshake_io.cpp |
Moderate (2 votes): interrupted reads should retry on EINTR. |
src/brpc/handshake/handshake_frame.h |
Handshake frame specifications. |
src/brpc/handshake/handshake_frame.cpp |
Handshake frame encoding and parsing. |
src/brpc/handshake/handshake_adapter.h |
Handshake adapter interfaces. |
src/brpc/handshake/handshake_adapter.cpp |
Handshake/input-messenger bridge. |
src/brpc/global.cpp |
Handshake protocol registration. |
src/brpc/adapter_transport.h |
Adapter transport interface. Critical (1 vote): capability checks must be transport-mode-specific before concrete downcasts. |
src/brpc/adapter_transport.cpp |
Common orchestration and fallback. Moderate (1 vote): post-upgrade UBSHM TCP data needs rejection handling. Moderate (1 vote): StopConnect must cancel blocked handshake work. |
Makefile |
Build-source integration. |
docs/cn/handshake_common_design.md |
Common handshake design documentation. |
CMakeLists.txt |
Source and protobuf integration. |
BUILD.bazel |
Bazel source integration. |
Review details
Suppressed comments (8)
src/brpc/adapter_transport.cpp:340
- For a server UBSHM socket this callback remains registered on the TCP control fd after the handshake, but it directly invokes
InputMessenger::OnNewMessagesin every phase. Once the upgrade is established, TCP is only the control channel and application data should arrive from UBRing; a later TCP write can otherwise be parsed and dispatched as an ordinary RPC (or re-enter the handshake parser) instead of being rejected. Use the same post-upgrade unexpected-TCP check as the RDMA server path while retaining normal parsing during TCP fallback.
_on_edge_trigger = InputMessenger::OnNewMessages;
src/brpc/adapter_transport.cpp:62
StartConnectlaunchesProcessClientHandshake, whose task owns aSocketUniquePtr, but thisStopConnectimplementation is a no-op. If the socket is failed while the handshake is blocked inSocketHandshakeIO::ReadExact, nothing wakes the handshake's read butex or cancels the bthread; the task keeps the socket referenced, so recycling cannot reachStopConnectand the connection can leak a bthread/reference indefinitely. Keep a cancellable handshake handle (or make the handshake I/O observe failure and wake/abort) and cancel it here.
void StopConnect(Socket*) override {}
src/brpc/handshake/handshake_io.cpp:117
write(2)is also allowed to returnEINTR, but this loop immediately reports it as a fatal error. Retry interrupted writes before theEAGAINwait path, otherwise a signal during the handshake can spuriously fail the connection.
if (errno != EAGAIN) {
return -1;
}
src/brpc/handshake/rdma_handshake.cpp:551
- As with UBSHM, the ACK parser may leave application bytes in
sourcewhen the peer coalesces them with the handshake. This check turns that case into a failed RDMA connection even thoughStandardHandshakeAdapteris designed to returnPARSE_ERROR_TRY_OTHERSand let the normal protocol parser consume the remaining buffer. Validate the transport state without rejecting buffered application data.
if (!source->empty()) {
return STEP_ERROR;
}
src/brpc/handshake/ubshm_handshake.cpp:369
HandshakeSession::RunServerinvokesvalidate_established()beforeset_high_speed_active(). ConsequentlyUpgradeActive()is still false here on every otherwise valid UBSHM handshake, so this callback returnsSTEP_ERRORand the session never reachesESTABLISHED. Remove this pre-activation check or move validation to a point where activation is already published.
if (!source->empty() ||
!transport->UpgradeActive()) {
src/brpc/handshake/ubshm_handshake.cpp:370
- The ACK has already been consumed when this validation runs, but any application bytes coalesced in the same TCP read remain in
source. ReturningSTEP_ERRORrejects that valid stream instead of returningTRY_OTHERSsoInputMessengerProcessorcan parse the remaining bytes. The validator should not require the input buffer to be empty.
if (!source->empty() ||
!transport->UpgradeActive()) {
return STEP_ERROR;
src/brpc/rdma_transport.cpp:48
- If
new (std::nothrow)fails, this branch marks the socket failed but continues initialization with_rdma_ep == nullptr.RdmaTransportremains upgrade-capable, so later handshake/resource calls dereference the null endpoint (andGetRdmaEp()can hitCHECK). Propagate the initialization failure through the socket creation path or make the transport unavailable before returning.
src/brpc/ubshm_transport.cpp:53 - This allocation failure path records
SetFailedbut still returns fromInitwith_ub_ep == nullptrand a non-null high-speed transport. A subsequent handshake can callGetUBShmEp()/resource methods and dereference the missing endpoint. Propagate initialization failure or mark the transport unavailable before returning instead of leaving a partially initialized upgrade path.
- Files reviewed: 43/44 changed files
- Comments generated: 8
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 43 out of 44 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
src/brpc/rdma_transport.cpp:1
- Failing the Socket in
RdmaTransport::InitwhenRdmaEndpointallocation fails prevents the intended TCP fallback path from working (the connection becomes unusable even though the PR’s design explicitly supports fallback). Instead ofsocket->SetFailed(...)here, treat this as 'upgrade unavailable' (e.g., mark RDMA as OFF/disabled and letAdapterTransportcontinue with TCP), and ensure later RDMA-only operations are gated on upgrade readiness.
src/brpc/ubshm_transport.cpp:1 - Same issue as RDMA: marking the Socket failed on
UBShmEndpointallocation failure breaks TCP fallback (connection becomes unusable). This should be handled as 'UBSHM upgrade not available' and allowAdapterTransportto proceed with TCP, rather than failing the socket during init.
src/brpc/ubshm/ub_endpoint.cpp:1 - These UBSHM/UBRing flags have RDMA-specific descriptions ("RDMA polling mode" / "Disable bthread in RDMA"), which is misleading for operators. Update the strings to reference UBRing/UBSHM polling mode to match the flag names and file context.
wwbmmm
left a comment
wwbmmm
left a comment
There was a problem hiding this comment.
[replied by brpc-oncall robot] The handshake refactor looks reasonable and the latest revision addresses the earlier upgrade-selection, synchronization and resource-release concerns, but an accidental logic drop in RdmaEndpoint and the unrelated 2-space reformatting should be fixed.
| int fd = _resource->comp_channel->fd; | ||
| GetGlobalEventDispatcher(fd, _socket->_io_event.bthread_tag()).RemoveConsumer(fd); | ||
| GetGlobalEventDispatcher(fd, _socket->_io_event.bthread_tag()) | ||
| .RemoveConsumer(fd); |
There was a problem hiding this comment.
[replied by brpc-oncall robot] The inner if (_cq_sid != INVALID_SOCKET_ID) guard was dropped during the reformatting. RemoveConsumer(comp_channel->fd) is now called even when this endpoint never registered that fd with the event dispatcher, i.e. when DeallocateResources() runs after AllocateResources() but before StartCqEvents() (which is exactly what happens on TCP fallback via DeactivateUpgrade() -> RdmaEndpoint::Reset()). Calling epoll_ctl(EPOLL_CTL_DEL) on an fd that was never added fails with ENOENT and logs misleading "Fail to remove fd" warnings. Please restore the _cq_sid guard (and keep remove_consumer as before).
There was a problem hiding this comment.
Fixed. The _cq_sid != INVALID_SOCKET_ID guard has been restored before removing the RDMA completion-channel consumer.
| return _default_connect; | ||
| } | ||
|
|
||
| void RdmaTransport::SetHighSpeedAvailable(bool available) { |
There was a problem hiding this comment.
[replied by brpc-oncall robot] This new code uses 2-space indentation while the rest of the file/repo follows the 4-space style, and the PR reindents many unrelated blocks (rdma_transport.cpp, rdma_endpoint.cpp, ub_endpoint.cpp). Please keep 4-space indentation and separate pure reformatting from functional changes, so the actual logic diffs stay reviewable and accidental drops such as the _cq_sid guard above are not buried in whitespace.
There was a problem hiding this comment.
Fixed. The affected RDMA and UBSHM transport/endpoint code has been restored to 4-space indentation, and the accidental formatting noise was cleaned up.
There was a problem hiding this comment.
🟡 Changes recommended
Four unresolved review findings remain, including two critical issues.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
src/brpc/adapter_transport.cpp:432
- When endpoint allocation fails,
upgrade_capable(_mode)is false, so this returns the raw TCP connector without running the common session and leaves the phaseUNINITIALIZED. Client high-speed sockets useOnNewDataFromTcp, whoseProcessTcpEventdoes not invokeInputMessengerin that phase, so TCP fallback responses are never parsed. Install the normal input callback for this no-upgrade client path (after the high-speed transport is initialized), or publishFALLBACK_TCPbefore returning.
std::shared_ptr<AppConnect> AdapterTransport::Connect() {
if (upgrade_capable(_mode)) {
return std::make_shared<AdapterConnect>(_default_connect);
}
return _tcp_transport->Connect();
src/brpc/adapter_transport.cpp:430
_app_connectis copied intoSocketOptions.app_connectwhen the channel creates pooled/short sockets (socket.cpp:2805and2908). After the first high-speed socket, that value is already anAdapterConnect, so this branch wraps it in a secondAdapterConnect; the outer handshake then invokes the inner one, causing a second handshake/resource allocation on an already-upgraded endpoint (RdmaEndpoint::DoAllocateResourcesasserts_resource == nullptr, and UBSHM has the analogous check). Return an existing adapter (and leave null server-side app-connects null) instead of nesting the wrapper.
std::shared_ptr<AppConnect> AdapterTransport::Connect() {
if (upgrade_capable(_mode)) {
return std::make_shared<AdapterConnect>(_default_connect);
src/brpc/handshake/rdma_handshake.cpp:560
- The common driver invokes
on_failedfor send, ACK, and protocol errors after RDMA resources may already have been allocated, but this callback is a no-op. The failed session can therefore retain its QP/CQ resources until socket recycling; deactivate the upgrade here so the failed state also releases transport resources and stops transport-side activity.
callbacks.transport.on_failed = []() {};
src/brpc/handshake/ubshm_handshake.cpp:385
- The common driver invokes
on_failedfor send, ACK, and protocol errors after the UBRING resources and poller may already have been created, but this callback is a no-op. The failed session can retain the ring/poller until socket recycling; deactivate the upgrade here so failure immediately tears down transport-side resources.
callbacks.transport.on_failed = []() {};
- Files reviewed: 43/44 changed files
- Comments generated: 2
- Review effort level: Lite
| CHECK_EQ(_mode, options.socket_mode); | ||
| _socket = socket; | ||
| _default_connect = options.app_connect; | ||
| _on_edge_trigger = options.on_edge_triggered_events; |
| strncpy(remote_trx_shm.name, remote.shm_name, | ||
| SHM_MAX_NAME_BUFF_LEN); |
wwbmmm
left a comment
wwbmmm
left a comment
There was a problem hiding this comment.
[replied by brpc-oncall robot] Copilot's two critical findings are valid and must be fixed (AdapterConnect must not be re-wrapped for pooled/short sockets, and the server must reject a non-NUL-terminated remote.shm_name before strncpy/strlen), but the client edge-trigger race claim does not apply to the sockets this PR creates.
| CHECK_EQ(_mode, options.socket_mode); | ||
| _socket = socket; | ||
| _default_connect = options.app_connect; | ||
| _on_edge_trigger = options.on_edge_triggered_events; |
There was a problem hiding this comment.
[replied by brpc-oncall robot] This line does not leave InputMessenger::OnNewMessages active for client high-speed sockets, so I don't think the claimed race with ProcessClientHandshake's blocking reads exists. Client sockets are created through InputMessenger::Create(SocketOptions, SocketId*) (Channel via SocketMap, and Socket::GetPooledSocket/GetShortSocket), which only sets need_on_edge_trigger = true and leaves options.on_edge_triggered_events NULL; the overload that assigns OnNewMessages has no callers. For a client high-speed socket options.user == get_client_side_messenger(), so this branch falls through to _on_edge_trigger = OnNewDataFromTcp, which only calls NotifyReadable() and never reads the fd. Pooled/short sockets propagate the same NULL value, so they also get OnNewDataFromTcp. Please double-check whether any caller sets on_edge_triggered_events for these sockets before changing this.
* fix: harden transport fallback and UBSHM validation * fix: distinguish uninitialized client messenger * test: initialize client messenger for fallback coverage
|
please resolve conflicts |
wwbmmm
left a comment
wwbmmm
left a comment
There was a problem hiding this comment.
This PR was intended to reduce duplicated code logic, but actually it added more lines of code than removed code.
| TransportUpgradeOps -> transport-specific endpoint/resource | ||
| ``` | ||
|
|
||
| 禁止以下反向依赖： |
There was a problem hiding this comment.
Thanks for the feedback. We’re looking into this and will continue simplifying the common handshake design to remove unnecessary complexity and duplication. We’ll update the PR once the changes are ready.
There was a problem hiding this comment.
Updated. We reworked the common handshake abstraction and replaced the callback-based compatibility layer with explicit HandshakeProtocol and HandshakeTransport participant interfaces.
This removes the callback conversion wrappers and makes the protocol-specific wire handling and transport resource lifecycle responsibilities explicit. The design document has also been updated accordingly.
Could you please take another look when you have time?
| } | ||
| cntl->response_attachment().append(cntl->request_attachment()); | ||
| } | ||
| void Echo(google::protobuf::RpcController *cntl_base, |
There was a problem hiding this comment.
Do not change the code indentation from 4 to 2.
There was a problem hiding this comment.
Thanks for pointing this out. We’re cleaning up the indentation and unrelated formatting changes as part of the current update.
There was a problem hiding this comment.
Fixed. The RDMA test code has been restored to the existing 4-space indentation, and the unrelated formatting changes have been removed.
refactor: introduce transport handshake participants
What problem does this PR solve?
Issue Number: N/A
Related Discussion: #3432
Problem Summary:
RDMA, UBSHM, and URMA use similar connection setup flows: establish a TCP control connection, exchange handshake messages, prepare transport-specific resources, negotiate whether the high-speed transport can be used, and fall back to TCP when necessary.
These handshake flows were previously implemented separately in individual transports, resulting in duplicated framing, TCP handshake I/O, state transitions, fallback handling, and connection orchestration.
As discussed in the related Discussion, the handshake orchestration should be moved above individual high-speed transports, while transport-specific resource management and data-plane operations remain inside each transport.
This PR introduces the common transport-level handshake framework and migrates both RDMA and UBSHM to it. URMA can be migrated to the same framework in a follow-up change.
What is changed and the side effects?
Changed:
AdapterTransportas the top-level transport for TCP, RDMA, and UBSHM sockets.RdmaEndpoint.UBShmEndpoint.rdma_handshakeprotocol name.Side effects:
Performance effects:
Breaking backward compatibility:
Check List: