zchuango commented on code in PR #3573:
URL: https://github.com/apache/brpc/pull/3573#discussion_r4143778571
##########
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:
Agreed that this failure path needs explicit cleanup. The specific “fewer
than two slots” example may not be reachable through the normal IPC allocation
path because shared memory is required to be at least 4 MiB and allocation-unit
aligned, but the error path should still release all provisional resources.
I’ll address it after updating this PR with #3509 and add failure-path coverage.
##########
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:
Agreed. The format-mismatch fallback needs explicit rollback of the
provisional server resources. This overlaps with #3509, which changes timer
cancellation, cleanup ownership, and transaction slot reuse. I plan to update
this PR after #3509 lands, then implement the rollback using the new lifecycle
APIs and add coverage for this fallback path.
--
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]
