Skip to content

Add UBRing data format negotiation - #3507

Open
zchuango wants to merge 1 commit into
apache:masterfrom
LinQuickDev:ubshm_transport_fix_3-1
Open

Add UBRing data format negotiation#3507
zchuango wants to merge 1 commit into
apache:masterfrom
LinQuickDev:ubshm_transport_fix_3-1

Conversation

@zchuango

Copy link
Copy Markdown
Contributor

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:

  • PR1 adds an explicit and backward-safe UBRing data format negotiation mechanism.
  • A follow-up PR will introduce an IPC-specific data format and optimize the IPC data path.

UBRing currently uses the same legacy data format for both IPC and UBS, but the handshake only negotiates hello_ver and impl_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:

  • Bump the UBRing hello protocol version from V2 to V3 while keeping the base Hello wire format unchanged at 64 bytes.
  • Add UBR_DATA_FORMAT_NONE and UBR_DATA_FORMAT_LEGACY_64.
  • Add a separate fixed-size 4-byte format extension containing a 16-bit length and a 16-bit format identifier, both serialized in network byte order.
  • Exchange the format extension only after both peers confirm V3 compatibility through the base Hello.
  • Let the client propose LEGACY_64, and let the server select LEGACY_64 or NONE.
  • Map remote shared memory only after a supported, matching, non-NONE format is selected.
  • Send ACK=0 and fall back to TCP when format negotiation fails.
  • Do not exchange format-extension bytes between V2 and V3 peers.
  • Require the base Hello msg_len to be exactly 64 so that unsupported extra Hello bytes cannot remain unread in the TCP stream.
  • Record and reset the negotiated data format in UBShmEndpoint.
  • Add focused tests for serialization/deserialization, network byte order, NONE, unknown format values, and negotiated-format state cleanup.
  • Keep the existing UBRing message layout, 64-byte slot, 60-byte payload, 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:

bazel test --config=ubring //test:brpc_ubring_unittest
15 tests from 4 test suites passed.

Smoke-test results:

  • V3 client + V3 server negotiated LEGACY_64 and completed RPC requests.
  • V3 client + baseline V2 server fell back to TCP and completed 10 RPC requests.
  • Baseline V2 client + V3 server fell back to TCP and completed 10 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:

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 UbrDataFormat and HelloFormatExtension (network-byte-order serialized) and tracks the negotiated format in UBShmEndpoint.
  • 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)
@zchuango

Copy link
Copy Markdown
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?
I checked the existing code and found that this behavior is not specific to the new format negotiation. The existing version-mismatch and remote-mapping-failure paths also keep the resources until the socket is destroyed.
Simply calling DeallocateResources() on fallback may not be safe, since it calls UbrTrxClose() and may wait for a peer even though the UBRing connection was never established.
Do you think this should be addressed in this PR, or would a separate cleanup PR covering all handshake fallback paths be more appropriate?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants