Repository navigation
Add UBRing data format negotiation - #3507
Merged
Merged
Conversation
Contributor
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.
Comment on lines
+675
to
680
| 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)); |
Comment on lines
500
to
504
| 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) |
Contributor
Author
|
@wwbmmm @chenBright Could you help confirm the scope of the two Copilot comments about UB resources not being released immediately after TCP fallback? |
Contributor
I think you can ignore these Copilot comments. |
Contributor
|
LGTM |
Contributor
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
Contributor
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
Comment on lines
+465
to
471
| 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; |
Contributor
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;
Comment on lines
+439
to
+461
| 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; |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There is a confirmed portability/UB issue when parsing ACK flags via an unaligned uint32_t* cast, and the new handshake paths are not yet unit-tested end-to-end.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
src/brpc/ubshm/ub_endpoint.cpp:626
- PR description says remote shared memory is mapped only after a supported, matching, non-NONE format is selected, but on the server side the SHM allocation/mapping happens before this format-extension exchange (via AllocateServerResources → UbrAllocateServerShm/UbrServerTrxInit). If the intent is to avoid mapping when format negotiation fails, consider deferring server resource allocation until after format selection, or clarify the description to specify this guarantee is client-side only.
UbrDataFormat selected_format = UBR_DATA_FORMAT_NONE;
if (HelloNegotiationValid(remote_msg) &&
HelloNegotiationValid(local_msg)) {
ep->_state = S_FORMAT_WAIT;
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
Comment on lines
672
to
673
| uint32_t* tmp = (uint32_t*)data; | ||
| uint32_t flags = butil::NetToHost32(*tmp); |
Comment on lines
+65
to
+69
| TEST(HelloFormatExtensionTest, serialize_deserialize_roundtrip) { | ||
| brpc::ubring::HelloFormatExtension extension = { | ||
| brpc::ubring::HelloFormatExtension::WIRE_SIZE, | ||
| brpc::ubring::UBR_DATA_FORMAT_LEGACY_64}; | ||
| char buffer[brpc::ubring::HelloFormatExtension::WIRE_SIZE] = {}; |
Contributor
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 4 comments.
Comment on lines
+437
to
+441
| HelloFormatExtension local_extension = { | ||
| HelloFormatExtension::WIRE_SIZE, UBR_DATA_FORMAT_LEGACY_64}; | ||
| local_extension.Serialize(data); | ||
| ep->_state = C_FORMAT_SEND; | ||
| if (ep->WriteToFd(data, HelloFormatExtension::WIRE_SIZE) < 0) { |
Comment on lines
+463
to
+469
| HelloFormatExtension remote_extension; | ||
| remote_extension.Deserialize(data); | ||
| 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(); |
Comment on lines
423
to
424
| LOG(WARNING) << "Fail to parse Hello Message length from server:" | ||
| << s->description(); |
Comment on lines
+52
to
+54
| // 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; |
Contributor
|
LGTM |
chenBright
reviewed
Sep 21, 2026
Sherlock0203
added a commit
to Sherlock0203/brpc
that referenced
this pull request
Oct 16, 2026
Bring the negotiation and IPC_V2 work that landed on master (apache#3507, apache#3573) into this branch so it can be merged back without conflicts. Those master changes were built on the timerfd-era close path, which this branch had replaced with the bthread timer facade, generation-checked pool teardown and the cleanup worker, so both sides evolved the same ubshm core after 9334389. Resolution: - Close/clear core (UbrTrxClose, UbrTrxCloseCallback, UbrClearResourceCheck, ClearTrxResource, UbrPassiveClearTrx, UbrTrxCloseCheck) keeps this branch's versions. Master's extra start_time/op parameters were unused, and UbrCloseType only chose between DeleteTimerSafe and DeleteTimer, which the facade subsumes: UbrTimerDelAndWait degrades to the non-blocking delete inside a callback. The unused UbrCloseType enum is therefore dropped. - Master's format-aware initialization is kept: UbrServerTrxInit keeps the UbrDataFormat parameter and the Initialize*LegacyFormat split, UbrAllocateServerShm/UbrMapRemoteShm(UbrMapRemoteShmAddTimer) keep passing it, and ApplyAndMapLocalShm no longer presets the rx capacity (InitializeClientLocalLegacyFormat owns that now). All IPC_V2 ring code and the negotiation handshake are master's, unchanged. - Master-side UNLIKELY/LIKELY are converted to BAIDU_UNLIKELY/ BAIDU_LIKELY, matching the module convention from this branch's style pass; common.h keeps the plain aliases. - This branch's fixes that master lacked merge on top: the poller re-ADD refresh, butil::atomic for the poller-group flag, the lazy bthread timers replacing TimerInit, and the cleanup-worker changes (leaked queue, start retry instead of the terminal latch). - test/brpc_ubring_unittest.cpp: master's IPC_V2 assertions on the removed timer_fd/hb_timer_fd fields now check the unarmed close_timer/hb_timer slots instead. The UbrCleanupWorkerTest that PR LinQuickDev#74's squash dropped is restored (the commit describes it and the g_ub_test_worker_start_rc hook it exercises, but only the two source files made it into the squash), so both sides' suites run: 31 tests. Tested: cmake --build build --target brpc_ubring_unittest (WITH_UBRING =ON, BUILD_UNIT_TESTS=ON) and ./build/test/brpc_ubring_unittest (31/31), also under ASAN (detect_leaks=0, 31/31). e2e with example/ubring_performance --use_ubring=true over the IPC backend: UBRING_* mappings confirmed, the negotiated IPC_V2 data path sustains ~148k QPS at ~6us, phase teardowns are clean, and a server kill -9 is handled without errors or residue.
Sherlock0203
added a commit
to Sherlock0203/brpc
that referenced
this pull request
Oct 19, 2026
Bring the apache master line up to f0239d7 into this branch in a single merge, replacing the two stacked merges with master@36c2d12e and the macOS fix only. Everything the branch needs from that line is included: the data-format negotiation (apache#3507), the IPC_V2 data path (apache#3573), the macOS OpenSSL 4.0 build fix (apache#3616: X509 accessors return const pointers in the current macos-26-arm64 runner image, fixed with auto*), and the newer fixes, notably the null-termination of the peer shm_name in HelloMessage::Deserialize (apache#3605), the MPSC queue node release ordering (apache#3620) and the MultiFlatMap bucket-head erasure fix (apache#3617). Those master changes were built on the timerfd-era close path, which this branch had replaced with the bthread timer facade, generation-checked pool teardown and the cleanup worker, so both sides evolved the same ubshm core after 9334389. Resolution: - Close/clear core (UbrTrxClose, UbrTrxCloseCallback, UbrClearResourceCheck, ClearTrxResource, UbrPassiveClearTrx, UbrTrxCloseCheck) keeps this branch's versions. Master's extra start_time/op parameters were unused, and UbrCloseType only chose between DeleteTimerSafe and DeleteTimer, which the facade subsumes: UbrTimerDelAndWait degrades to the non-blocking delete inside a callback. The unused UbrCloseType enum is therefore dropped. - Master's format-aware initialization is kept: UbrServerTrxInit keeps the UbrDataFormat parameter and the Initialize*LegacyFormat split, UbrAllocateServerShm/UbrMapRemoteShm(UbrMapRemoteShmAddTimer) keep passing it, and ApplyAndMapLocalShm no longer presets the rx capacity (InitializeClientLocalLegacyFormat owns that now). All IPC_V2 ring code and the negotiation handshake are master's, unchanged, with master-side UNLIKELY/LIKELY converted to the module's BAIDU_* macros. - This branch's fixes that master lacked merge on top: the poller re-ADD refresh, butil::atomic for the poller-group flag, the lazy bthread timers replacing TimerInit, and the cleanup-worker changes (leaked queue, start retry instead of the terminal latch). - test/brpc_ubring_unittest.cpp: master's IPC_V2 assertions on the removed timer_fd/hb_timer_fd fields check the unarmed close_timer/hb_timer slots instead, and the UbrCleanupWorkerTest that PR LinQuickDev#74's squash dropped is restored alongside master's apache#3605 hardening of the full_shm_name test. Both sides' suites run. Tested: the close/cleanup/IPC_V2 resolution was verified on the equivalent tree before apache#3605 came in -- cmake WITH_UBRING=ON build, 31/31 unit tests, the same under ASAN (detect_leaks=0), and an example/ubring_performance e2e over the IPC backend (negotiated IPC_V2 path at ~148k QPS, clean teardowns, server kill -9 handled). The new master commits are additive to that resolution; cmake --build build --target brpc_ubring_unittest and ./build/test/brpc_ubring_unittest pass on this exact tree as well.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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: