chenBright commented on code in PR #3535:
URL: https://github.com/apache/brpc/pull/3535#discussion_r4455646517
##########
src/brpc/ubshm/ub_ring.cpp:
##########
@@ -220,41 +221,9 @@ RETURN_CODE UBRing::UbrAddHBTimer() {
return UBRING_OK;
}
-RETURN_CODE UBRing::UbrPassiveClearTrx(UbrTrx *trx, int fd, PASSIVE_DISC_TYPE
type) {
- RETURN_CODE passive_close_check_rc = UbrTrxCloseCheck(trx);
- if (UNLIKELY(passive_close_check_rc != UBRING_OK)) {
- if (passive_close_check_rc == UBRING_REENTRY) {
- LOG(INFO) << "Passive close skipped, active close in progress,
name=" << trx->local_shm.name;
- uint64_t start_time = GetCurNanoSeconds();
- return ClearTrxResource(trx, start_time, UBR_CALL_BACK_CLOSE);
- }
- return UBRING_ERR;
- }
- trx->ubr_tx.trx_state = UBR_STATE_CLOSED;
- trx->ubr_rx.trx_state = UBR_STATE_CLOSED;
- DeleteTimerSafe((uint32_t)trx->timer_fd);
- const char *type_name = nullptr;
- if (type == UBR_HEARTBEAT) {
- DeleteTimer((uint32_t)trx->hb_timer_fd);
- type_name = "Trx heartbeat";
- } else if (type == UBR_UB_EVENT) {
- DeleteTimerSafe((uint32_t)trx->hb_timer_fd);
- type_name = "Ub event callback";
- }
- constexpr int64_t kMicrosecondsPerSecond = 1000000LL;
- bthread_usleep(FLAGS_ub_flying_io_timeout_s * kMicrosecondsPerSecond);
-
- int rc = ShmLocalFree(&trx->remote_shm);
- if (rc != UBRING_OK) {
- LOG(ERROR) << type_name << ", delete remote shm failed. ret=" << rc;
- }
- rc = ShmLocalFree(&trx->local_shm);
- if (rc != UBRING_OK) {
- LOG(ERROR) << type_name << ", delete local shm failed. ret=" << rc;
- }
-
- UBRingManager::ReleaseUbrTrxFromMgr(trx);
- return UBRING_OK;
+RETURN_CODE UBRing::UbrPassiveClearTrx(UbrTrx* trx) {
+ const RETURN_CODE rc = ClearTrxResource(trx, UBR_PASSIVE_CLOSE);
+ return rc == UBRING_REENTRY ? UBRING_OK : rc;
Review Comment:
After passive disconnection was changed to use `ClearTrxResource()`, it
eventually calls `UbrTrxFreeShm()`. That function returns immediately if the
local `ShmMunmap()` fails, so the remote `ShmLocalFree()` is not executed. For
example, when the UBS backend returns `UBSM_ERR_NET`, only the local SHM enters
the retry queue, while the asynchronous callback still releases the cleanup
reference. Afterward, because the state is already `UBR_CLOSE_SECOND`,
`UbrTrxClose()` only releases the ring reference and does not perform the
missed remote cleanup. Even if the local retry succeeds, cleanup of the remote
mapping is still missed. The original passive path attempted to free both sides
independently; please preserve that behavior and arrange retries for each
failed resource.
##########
src/brpc/ubshm/ub_ring.cpp:
##########
@@ -61,7 +61,44 @@ RETURN_CODE UBRing::UbrTrxMapShm(SHM *local_shm, SHM
*remote_shm)
return UBRING_OK;
}
+RETURN_CODE UBRing::UbrTrxAbort() {
+ if (_trx == nullptr) {
+ return UBRING_OK;
+ }
+ if (ATOMIC_LOAD(_trx->close_state) != UBR_CLOSE_FIRST) {
+ return ReleaseTrx();
+ }
+ const RETURN_CODE close_rc = UbrTrxCloseCheck(_trx);
+ if (close_rc != UBRING_OK) {
+ return close_rc;
+ }
+ int expected = UBR_CLOSE_FIRST;
+ if (!ATOMIC_COMPARE_EXCHANGE_STRONG(
+ _trx->close_state, expected, UBR_CLOSE_SECOND)) {
+ return ReleaseTrx();
+ }
+ // Drain timer callbacks before unmapping the queues they access.
+ DeleteTimerSafe(_trx->timer_fd);
+ DeleteTimerSafe(_trx->hb_timer_fd);
+ DeleteTimerSafe(_trx->clear_timer_fd);
+ RETURN_CODE rc = UbrTrxFreeShm(_trx);
+ if (rc == UBRING_OK) {
+ rc = ReleaseTrx();
+ }
Review Comment:
When using `SHM_TYPE_UBS` during an upgrade rollback, the `UBSM_ERR_NET`
branch in `ShmMunmap()` adds the SHM to the retry queue and causes
`UbrTrxFreeShm()` to return an error, so `ReleaseTrx()` is skipped here.
However, the caller, `UBShmEndpoint::Reset()`, still deletes the ring
afterward, and the transaction timer has already been canceled; the SHM retry
callback does not release the transaction reference. A single failure will
permanently occupy a transaction slot. As failures accumulate, UBSHM
transactions can no longer be allocated, and `UbrMgrFini()` will continue
waiting. Please explicitly transfer responsibility for retrying failed
resources and ensure that the transaction reference held by the ring is
eventually released.
--
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]
