feat: add common transport-level handshake for RDMA and UBSHM - #3535
feat: add common transport-level handshake for RDMA and UBSHM#3535zchuango wants to merge 2 commits into
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.
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: