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? |
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: