Copilot commented on code in PR #3573:
URL: https://github.com/apache/brpc/pull/3573#discussion_r4142435720
##########
src/brpc/ubshm/ub_endpoint.cpp:
##########
@@ -636,10 +667,13 @@ void* UBShmEndpoint::ProcessHandshakeAtServer(void* arg) {
remote_extension.Deserialize(data);
HelloFormatExtension local_extension = {
HelloFormatExtension::WIRE_SIZE, UBR_DATA_FORMAT_NONE};
- if (remote_extension.extension_len == HelloFormatExtension::WIRE_SIZE
&&
- remote_extension.format_id == UBR_DATA_FORMAT_LEGACY_64) {
- local_extension.format_id = UBR_DATA_FORMAT_LEGACY_64;
- selected_format = UBR_DATA_FORMAT_LEGACY_64;
+ if (remote_extension.extension_len == HelloFormatExtension::WIRE_SIZE)
{
+ selected_format =
+ SelectDataFormat(local_format, remote_extension.format_id);
+ }
+ if (selected_format != UBR_DATA_FORMAT_NONE) {
+ local_extension.format_id =
+ static_cast<uint16_t>(selected_format);
} else {
ub_transport->_ub_state = UBShmTransport::UB_OFF;
Review Comment:
When a V3 peer proposes `LEGACY_64` to an IPC server,
`AllocateServerResources()` has already mapped both queues, registered the
poller, started timers, and marked the `IPC_V2` transaction connected. Setting
`UB_OFF` here leaves those resources active for a TCP-only connection. The
heartbeat can eventually release the pooled transaction while this endpoint's
ring and poller still hold its pointer; a reused slot could then be accessed as
another connection. After completing the fallback handshake, abort the
provisional transaction: unregister the poller, cancel its timers, release both
mappings and the manager slot, and detach the ring without waiting for peer
UBRing participation.
##########
src/brpc/ubshm/ub_ring.cpp:
##########
@@ -896,6 +1209,22 @@ RETURN_CODE UBRing::UbrMapRemoteShmAddTimer(SHM
*local_trx_shm, const char *loca
LOG(ERROR) << "Connect Trx map shared memory failed, remote shm=" <<
remote_trx_shm.name;
return rc;
}
+ if (format == UBR_DATA_FORMAT_LEGACY_64) {
+ InitializeClientRemoteLegacyFormat();
+ rc = InitializeClientLocalLegacyFormat();
+ if (LIKELY(rc == UBRING_OK)) {
+ InitializeClientLegacyFormat();
+ }
+ } else if (format == UBR_DATA_FORMAT_IPC_V2) {
+ rc = UbrPrepareIpcV2Format() == 0 ? UBRING_OK : UBRING_ERR;
+ } else {
+ rc = UBRING_ERR;
+ }
+ if (UNLIKELY(rc != UBRING_OK)) {
+ LOG(ERROR) << "Initialize client data format failed, local_name="
+ << local_name << ", format=" << format;
+ return rc;
Review Comment:
If the mapped peer queue fails `UbrPrepareIpcV2Format()` (for example, it
has fewer than two aligned slots), this return leaves the client's local and
remote shared-memory mappings and manager transaction allocated. No timer has
started, so a TCP fallback on a long-lived socket keeps those resources
indefinitely rather than reaching heartbeat cleanup. On this initialization
failure, abort setup and release both mappings and the transaction (and remove
the provisional poller registration) before sending ACK=0; do not use the
normal close path, which waits for a connected peer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
