Add UBRing data format negotiation - #3507
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an explicit, backward-safe UBRing data format negotiation step (Hello V3 + fixed 4-byte extension) so future IPC-specific formats can be introduced without corrupting the TCP stream when peers are on older versions.
Changes:
- Introduces
UbrDataFormatandHelloFormatExtension(network-byte-order serialized) and tracks the negotiated format inUBShmEndpoint. - Updates client/server handshakes to exchange the 4-byte format extension only after confirming Hello V3 compatibility and only maps remote shm when a supported format is selected (client side).
- Adds unit tests for extension serialization/deserialization and negotiated-format state reset.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/brpc_ubring_unittest.cpp | Adds focused tests for the new 4-byte format extension and negotiated-format state reset. |
| src/brpc/ubshm/ub_endpoint.h | Defines UbrDataFormat, HelloFormatExtension, and stores negotiated format in UBShmEndpoint. |
| src/brpc/ubshm/ub_endpoint.cpp | Implements extension (de)serialization, bumps Hello version to V3, and adds format negotiation steps to the handshake. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (ub_transport->_ub_state == UBShmTransport::UB_OFF || | ||
| selected_format == UBR_DATA_FORMAT_NONE) { | ||
| LOG(WARNING) << "Invalid successful ACK from client:" | ||
| << s->description(); | ||
| s->SetFailed(EPROTO, "Fail to complete ub handshake from %s: %s", | ||
| s->description().c_str(), berror(EPROTO)); |
| if (ub_transport->_ub_state == UBShmTransport::UB_ON) { | ||
| ep->_negotiated_data_format = selected_format; | ||
| ep->_state = ESTABLISHED; | ||
| ep->_ub_ring->UbrUnlinkLocalShm(); | ||
| LOG_IF(INFO, FLAGS_ub_trace_verbose) |
|
@wwbmmm @chenBright Could you help confirm the scope of the two Copilot comments about UB resources not being released immediately after TCP fallback? |
I think you can ignore these Copilot comments. |
|
LGTM |
There was a problem hiding this comment.
🔵 Needs a closer look
The new V3 negotiation increases fallback-to-TCP frequency, but fallback paths don’t reliably tear down already-created UBRing resources, risking persistent shm/poller overhead on TCP connections.
Review details
Suppressed comments (2)
src/brpc/ubshm/ub_endpoint.cpp:688
- If format negotiation fails (or other interop paths lead to ACK=0), the server falls back to TCP but retains any already-allocated UBRing resources from earlier in the handshake. With explicit V3 negotiation, this can be a normal path; keeping shm mappings and poller registrations alive for TCP connections wastes resources and keeps the UB poller scanning endpoints that will never establish UB.
ub_transport->_ub_state = UBShmTransport::UB_ON;
ep->_negotiated_data_format = selected_format;
ep->_state = ESTABLISHED;
ep->_ub_ring->UbrUnlinkLocalShm();
LOG_IF(INFO, FLAGS_ub_trace_verbose)
src/brpc/ubshm/ub_endpoint.cpp:504
- When the handshake ultimately falls back to TCP (e.g. V3<->V2 interop or format negotiation failure), the endpoint keeps the UBRing resources allocated earlier (local shm + poller sid + UBRing instance). After bumping hello_ver to 3, this fallback path becomes common and can leave many unused UB poller entries/shm segments alive, adding steady polling overhead and wasting shared-memory resources for connections that are actually TCP.
if (ub_transport->_ub_state == UBShmTransport::UB_ON) {
ep->_negotiated_data_format = selected_format;
ep->_state = ESTABLISHED;
ep->_ub_ring->UbrUnlinkLocalShm();
LOG_IF(INFO, FLAGS_ub_trace_verbose)
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
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 3 out of 3 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
src/brpc/ubshm/ub_endpoint.cpp:1
local_msg.hello_veris never set on the server side after zero-initializinglocal_msg. This will make the server sendhello_ver == 0, causingHelloNegotiationValid(local_msg)to fail and potentially forcing unintended fallback-to-TCP even when both peers are V3-capable. Setlocal_msg.hello_ver = g_ub_hello_version(as done on the client) before serializing/sending the hello.
// Licensed to the Apache Software Foundation (ASF) under one
| if (remote_extension.extension_len != HelloFormatExtension::WIRE_SIZE || | ||
| remote_extension.format_id == UBR_DATA_FORMAT_NONE || | ||
| remote_extension.format_id != local_extension.format_id) { | ||
| LOG(WARNING) << "Fail to negotiate data format with server, " | ||
| << "fallback to tcp:" << s->description(); | ||
| ub_transport->_ub_state = UBShmTransport::UB_OFF; | ||
| } else { |
| struct HelloFormatExtension { | ||
| // The V3 format extension is a fixed-size frame. A different wire size | ||
| // requires negotiation through a new hello version. | ||
| static const uint16_t WIRE_SIZE = 4; |
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 3 out of 3 changed files in this pull request and generated 3 comments.
Suppressed comments (1)
src/brpc/ubshm/ub_endpoint.cpp:427
- The log/error text says “Fail to parse Hello Message length…”, but this condition now triggers for any
msg_len != 64(including valid parse but unsupported/invalid length). Consider updating the message to “Invalid/unsupported Hello message length” and include the observedremote_msg.msg_lenvalue for easier diagnosis (same issue exists on the server-sidemsg_len != HELLO_MSG_LEN_MINcheck).
if (remote_msg.msg_len != HELLO_MSG_LEN_MIN) {
LOG(WARNING) << "Fail to parse Hello Message length from server:"
<< s->description();
s->SetFailed(EPROTO, "Fail to complete ubring handshake from %s: %s",
s->description().c_str(), berror(EPROTO));
ep->_state = FAILED;
| local_extension.Serialize(data); | ||
| ep->_state = C_FORMAT_SEND; | ||
| if (ep->WriteToFd(data, HelloFormatExtension::WIRE_SIZE) < 0) { | ||
| const int saved_errno = errno; | ||
| PLOG(WARNING) << "Fail to send format extension to server:" | ||
| << s->description(); | ||
| s->SetFailed(saved_errno, | ||
| "Fail to complete ubring handshake from %s: %s", | ||
| s->description().c_str(), berror(saved_errno)); | ||
| ep->_state = FAILED; | ||
| return nullptr; | ||
| } | ||
|
|
||
| ep->_state = C_FORMAT_WAIT; | ||
| if (ep->ReadFromFd(data, HelloFormatExtension::WIRE_SIZE) < 0) { | ||
| const int saved_errno = errno; | ||
| PLOG(WARNING) << "Fail to read format extension from server:" | ||
| << s->description(); | ||
| s->SetFailed(saved_errno, | ||
| "Fail to complete ubring handshake from %s: %s", | ||
| s->description().c_str(), berror(saved_errno)); | ||
| ep->_state = FAILED; | ||
| return nullptr; |
| ep->_state = FAILED; | ||
| return nullptr; | ||
| } | ||
| HelloFormatExtension remote_extension; |
| struct HelloFormatExtension { | ||
| // The V3 format extension is a fixed-size frame. A different wire size | ||
| // requires negotiation through a new hello version. | ||
| static const uint16_t WIRE_SIZE = 4; |
What problem does this PR solve?
Issue Number: #3463
Problem Summary:
This is Phase 3 PR1 of Issue #3463.
Phase 3 is split into two parts:
UBRing currently uses the same legacy data format for both IPC and UBS, but the handshake only negotiates
hello_verandimpl_ver. Before a follow-up PR can introduce an IPC-specific format, both peers must explicitly agree on the same data format.The existing base Hello is already 64 bytes. Directly appending extension bytes is unsafe because an old peer may consume only 64 bytes and leave the extra bytes in the TCP stream, corrupting the following ACK or application data.
This PR only establishes the format-negotiation foundation. It does not introduce the new IPC data format or change the existing UBRing data path.
What is changed and the side effects?
Changed:
UBR_DATA_FORMAT_NONEandUBR_DATA_FORMAT_LEGACY_64.LEGACY_64, and let the server selectLEGACY_64orNONE.NONEformat is selected.msg_lento be exactly 64 so that unsupported extra Hello bytes cannot remain unread in the TCP stream.UBShmEndpoint.NONE, unknown format values, and negotiated-format state cleanup.Copy64Byte, memory ordering, and send/receive data path unchanged.The V3 format extension is intentionally fixed at 4 bytes. A different extension wire size requires negotiation through a future hello version.
Tests:
Smoke-test results:
LEGACY_64and completed RPC requests.Side effects:
Performance effects:
V3-to-V3 connections add one fixed 4-byte request, one fixed 4-byte response, and one handshake round trip. The established UBRing data path has no additional per-message overhead.
Breaking backward compatibility:
No unsafe wire compatibility break is introduced. V2 and V3 peers intentionally fall back to TCP without exchanging format-extension bytes. TCP RPC traffic remains functional.
Check List: