Repository navigation
Ubshm transport fix 2 - #58
Open
Sherlock0203 wants to merge 14 commits into
Open
Sherlock0203 wants to merge 14 commits into
Sherlock0203 wants to merge 14 commits into
Conversation
Sherlock0203
force-pushed
the
ubshm_transport_fix_2
branch
from
September 24, 2026 07:31
72748e5 to
68a6914
Compare
…inQuickDev#57) Make the whole ubring shared-memory module follow the current brpc conventions, addressing review feedback: - Replace the module-local LIKELY/UNLIKELY aliases with BAIDU_LIKELY/ BAIDU_UNLIKELY and drop the aliases from common.h. - Use BAIDU_SCOPED_LOCK instead of the local GNU cleanup-attribute LOCK_GUARD. The pthread_mutex_t members keep their type, allocation and lifetime, so no static-init or placement-new change is needed. - Use butil::atomic instead of std::atomic. The timer handle slots and the trx cleanup slot are now real butil::atomic objects, so plain storage is never reinterpreted as an atomic: the timer API takes butil::atomic<UbrTimerId>* and UbrTrx/UbrCleanupCtl hold atomic handles. UbrTrx slots are value-initialized with placement new so those atomics are constructed, replacing the per-acquisition memset. - Drop the two per-node INFO logs on the shm cleanup retry path in UbsShmCallback; DeleteShmToList still records each drained node. No behavioral change: the removed aliases expanded 1:1, BAIDU_SCOPED_LOCK locks the same pthread_mutex_t, butil::atomic is layout-identical to std::atomic, and placement-new value-initialization zeroes exactly what memset zeroed while also constructing the atomic members.
Sherlock0203
force-pushed
the
ubshm_transport_fix_2
branch
2 times, most recently
from
September 29, 2026 06:57
14450e2 to
48e403b
Compare
Address the ubring lifecycle review on the bthread-timer refactor. Generation validation: - UbrTimerStart/UbrTimerTask carry an opaque `gen' and pass it back to the callback on every fire, so a callback armed for an object that was later released and reused can detect that it is stale. `gen' is required and sits before the optional backoff; the UBS shm timer passes 0 explicitly. - UbrScheduleClearTimer takes expect_ubr_id, rejects a mismatch before creating the cleanup ctl, and stamps ctl->ubr_id with it instead of reading the slot's current generation. - UbrTrxCallbackCheck runs before the generation check in the close and heartbeat callbacks, so trx is null-checked before trx->ubr_id is dereferenced; UbrTrxClose and UbEventCallback pass the generation they observed for the slot. - UbrPassiveClearTrx/ClearTrxResource/UbrAddAsynClearTimer thread expect_ubr_id through. Teardown: - ReleaseUbrTrxFromMgr disarms the per-acquisition close/heartbeat timers after the generation check and before the slot is marked FREE, so a rollback no longer orphans a periodic task onto the slot's next occupant. - UbrMgrFini snapshots the USED slots under g_ubr_trx_mgr_mtx and UbrTimerDelAndWait's their timers outside the lock before the pool is freed; the previous loop only waited for cleanup-ctl callbacks, so a dispatched close/heartbeat callback could still be reading UbrTrx when FREE_PTR(trx_mgr) ran. The walk is guarded by "all four pool arrays are allocated", which also stops the ctl loops from reading arrays that UbrMgrInit's allocation-failure path leaves uninitialized. - UbrClearResourceCheck/UbrPassiveClearTrx stop the per-trx timers through UbrStopTrxTimer, which waits with UbrTimerDelAndWait unless the caller is the timer callback itself (in_timer_callback), so teardown from a user thread or from the ubsmem fault callback cannot clear a trx whose callback is mid-flight. UbrTrxCloseCallback re-validates its event queue pointers after the generation check, like UbrTrxHBCallback, so a concurrent release cannot turn the dereference into a null access. The ordering argument is a synchronization one: bthread dispatches every timer callback from a single global timer thread, so two per-trx callbacks never run concurrently and the one-shot delayed-clear callback is ordered after them; ub_flying_io_timeout_s only delays the clear so in-flight IO can drain. Non-blocking UbrTimerDel is kept on the paths that hold g_ubr_trx_mgr_mtx (ReleaseUbrTrxFromMgr and the callback side), where the callbacks take the same lock and a wait would deadlock. Tested: cmake --build cmake-build-debug --target brpc_ubring_unittest, and ./test/brpc_ubring_unittest (12/12 passed).
Address the second ubring review round on the bthread-timer refactor. Timer facade: - A periodic callback that deletes its own timer now only marks the task stopped and leaves it anchored in the handle slot until the callback returns; UbrTimerOnFire retires the slot on exit. Previously the callback pulled the task out of the slot first, so a concurrent UbrTimerDelAndWait returned immediately and the caller could clear a trx whose callback was still running. - UbrTimerDelAndWait recognizes that the caller is the callback of that very timer and degrades to the non-blocking delete, which removes the in_timer_callback argument that every teardown entry point had to thread correctly (and could get wrong). - UbrTimerStart (one-shot) and UbrTimerStartPeriodic are now separate functions, and a periodic start rejects interval_us == 0 instead of silently arming a one-shot: that dropped the close check, the heartbeat or the shm drain for the rest of the connection's life. The close and hb call sites use the periodic form and validate their flags; the shm drain period is derived to stay positive because its flag legitimately accepts 0 for the one-shot delayed-clear. - A back-off function returning 0 keeps the previous interval: re-arming for "now" would let one timer monopolize the process-wide timer thread. - The two join waits poll every 100us instead of 1ms; they only run on teardown paths. Cleanup-control publication and trx close: - UBRingManager::TryClaimTrxClose validates the slot generation and moves close_cnt in one critical section, so a close started from a stale snapshot cannot mark the slot's new occupant CLOSED or delete its timers. UbEventCallback looks the faulty link up before it closes it, so this was reachable through the ubsmem fault callback; UbrTrxCloseCheck takes expect_ubr_id and is now only a wrapper around the claim. - TryPublishUnitCleanupCtl publishes the cleanup control object on the trx and anchors it in the pool slot under one lock, and DetachUnitCleanupCtl clears both, so a force close never observes a trx-side publication without its anchor. The inline fallback gates the cleanup on IsUbrTrxSlotUsed instead of ubr_id: ReleaseUbrTrxFromMgr does not change ubr_id, so the old check let a released slot be cleaned twice. Shutdown: - UbrMgrFini takes a shutdown barrier before anything else: acquisitions, timer arming (ArmTimersExclusive, now used by UbrAddTimer) and cleanup publications are refused, and the faulty-shm callback registers through BeginPoolAccess, which Fini waits for before the pool is freed. Because arming is serialized by the same lock, the timers armed before the flag are the complete set and one wait pass replaces the retry loop whose window was never actually closed. - The shm list is torn down through a handle: DestroyShmTimer takes ShmList** and clears it, so g_shm_list is never left dangling, a repeated teardown is a no-op and UbsShmFini is idempotent, which makes a repeated ShmMgrFini safe instead of a double free. UbrMgrInit value-initializes the pool instead of memset'ing it, matching the placement-new UbrTrx construction the style commit introduced (UbrTrx holds butil::atomic members). Tested: cmake --build cmake-build-debug --target brpc_ubring_unittest, and ./test/brpc_ubring_unittest (12/12 passed).
Sherlock0203
force-pushed
the
ubshm_transport_fix_2
branch
from
September 30, 2026 08:16
48e403b to
5f55f69
Compare
…inQuickDev#62) Address the second ubring review round on the bthread-timer refactor. Timer facade: - A periodic callback that deletes its own timer now only marks the task stopped and leaves it anchored in the handle slot until the callback returns; UbrTimerOnFire retires the slot on exit. Previously the callback pulled the task out of the slot first, so a concurrent UbrTimerDelAndWait returned immediately and the caller could clear a trx whose callback was still running. - UbrTimerDelAndWait recognizes that the caller is the callback of that very timer and degrades to the non-blocking delete, which removes the in_timer_callback argument that every teardown entry point had to thread correctly (and could get wrong). - UbrTimerStart (one-shot) and UbrTimerStartPeriodic are now separate functions, and a periodic start rejects interval_us == 0 instead of silently arming a one-shot: that dropped the close check, the heartbeat or the shm drain for the rest of the connection's life. The close and hb call sites use the periodic form and validate their flags; the shm drain period is derived to stay positive because its flag legitimately accepts 0 for the one-shot delayed-clear. - A back-off function returning 0 keeps the previous interval: re-arming for "now" would let one timer monopolize the process-wide timer thread. - The two join waits poll every 100us instead of 1ms; they only run on teardown paths. Cleanup-control publication and trx close: - UBRingManager::TryClaimTrxClose validates the slot generation and moves close_cnt in one critical section, so a close started from a stale snapshot cannot mark the slot's new occupant CLOSED or delete its timers. UbEventCallback looks the faulty link up before it closes it, so this was reachable through the ubsmem fault callback; UbrTrxCloseCheck takes expect_ubr_id and is now only a wrapper around the claim. - TryPublishUnitCleanupCtl publishes the cleanup control object on the trx and anchors it in the pool slot under one lock, and DetachUnitCleanupCtl clears both, so a force close never observes a trx-side publication without its anchor. The inline fallback gates the cleanup on IsUbrTrxSlotUsed instead of ubr_id: ReleaseUbrTrxFromMgr does not change ubr_id, so the old check let a released slot be cleaned twice. Shutdown: - UbrMgrFini takes a shutdown barrier before anything else: acquisitions, timer arming (ArmTimersExclusive, now used by UbrAddTimer) and cleanup publications are refused, and the faulty-shm callback registers through BeginPoolAccess, which Fini waits for before the pool is freed. Because arming is serialized by the same lock, the timers armed before the flag are the complete set and one wait pass replaces the retry loop whose window was never actually closed. - The shm list is torn down through a handle: DestroyShmTimer takes ShmList** and clears it, so g_shm_list is never left dangling, a repeated teardown is a no-op and UbsShmFini is idempotent, which makes a repeated ShmMgrFini safe instead of a double free. UbrMgrInit value-initializes the pool instead of memset'ing it, matching the placement-new UbrTrx construction the style commit introduced (UbrTrx holds butil::atomic members). Tested: cmake --build cmake-build-debug --target brpc_ubring_unittest, and ./test/brpc_ubring_unittest (12/12 passed).
…Dev#63) UbrTrxClose loaded _trx->ubr_id before calling UbrTrxCloseCheck, which is where the null check lived, so that guard became unreachable and the load dereferenced null. The path is reachable: UbrAllocateLocalShm and UbrAllocateServerShm reset _trx to nullptr on their error paths (a failing shm map, acquire or trx init) while the endpoint keeps the non-null _ub_ring, and UBShmEndpoint::DeallocateResources calls UbrTrxClose unconditionally from Reset(), so a connection whose setup failed crashes when its endpoint is destroyed, or when it falls back to TCP. Check _trx first and return UBRING_ERR, which restores the behavior from before the generation argument was added; DeallocateResources ignores the return value. UbrAddTimer takes the same guard: its failure branch reports _trx->local_shm.name before the guards in UbrAddCloseTimer/UbrAddHBTimer can reject a null trx. UbrTrxCloseCheck keeps its own null check: UbrPassiveClearTrx still routes through it. Tested: cmake --build cmake-build-debug --target brpc_ubring_unittest, and ./test/brpc_ubring_unittest (12/12 passed).
…eturns UbrTimerDel wins the handle-slot exchange and reports success for a one-shot timer, but a wrapper that the timer thread already dispatched may still be between its dispatch and its own (losing) slot exchange. The caller of UbrTimerDel treats return 0 as "the callback will not run" and frees the object that stores the slot -- UbrCleanupCtl on the delayed-clear path -- so that late wrapper access is a use-after-free. Track the slot retirement on the task: the one-shot wrapper sets slot_retired right after its slot CAS and before running the user callback, and UbrTimerDel waits for it when bthread_timer_del reports the task as dispatched. Callers that win the cancellation get the same guarantee from the delete itself. The wait only spins on a task-internal flag, so it is safe to do under the trx manager lock that the force-close, pool-acquire and UbrMgrFini callers hold, and it is limited to the one-shot path (a periodic task's slot storage is pool memory that the non-blocking callers never free).
The force close used to snapshot the pool slot's cleanup control object under the manager lock and then re-verify "still no ctl" out of the lock. A concurrent SDK-fault callback could publish a delayed cleanup for the same generation in between, so both the force close and the delayed-clear callback would run the cleanup of the same shared memory (double unmap / stale event queues). Replace the two steps with ClaimTrxCleanupForced, which snapshots the ctl and, when there is none, claims the cleanup for the generation inside one critical section, and make TryPublishUnitCleanupCtl refuse to publish once that claim happened (new per-acquisition trx->cleanup_forced flag, reset by the next AcquireUbrTrxFromMgr). Publication and claim are now totally ordered by the manager lock, so exactly one of them owns the cleanup. An existing publication keeps the current ctl->state arbitration untouched.
Timers are bthread based, so every callback in the process runs on one global timer thread. UbsShmCallback and the delayed-clear callbacks still called the UBS SDK there (ubsmem_shmem_unmap/deallocate), and those calls can wait on the SDK daemon with no latency bound, so a slow or failed shm release delayed every other timer in the process -- RPC timeouts, backup requests, bthread_usleep wakeups and ubring's own heartbeat/close checks. Add a lazily started cleanup worker bthread with an MPSC job queue. The timer callbacks now only arbitrate cleanup ownership and post a job (shared-memory drain or trx cleanup); the worker runs the blocking part, and takes over the timer/callback reference of the cleanup control object. Callers run the work inline only when the worker cannot be started. The drain keeps the previous retry semantics (the node stays in the list on unmap failure and the periodic timer retries it) and at most one drain step is in flight, and DestroyShmTimer drains the worker before it frees the list and finalizes the SDK. UbrMgrFini already waits for the control-object references, so it needs no change.
Sherlock0203
force-pushed
the
ubshm_transport_fix_2
branch
from
October 9, 2026 08:18
a345b29 to
e47d004
Compare
Post() was not serialized with the lazy start of the worker bthread. Two posters could decide "the worker is started" and "enqueue the job" independently: one thread won the start CAS and was about to see bthread_start_background fail, while another read g_worker_started == true and enqueued a job. Since g_worker_unavailable is terminal and no worker ever runs, that job is never dequeued -- its UbrCleanupCtl reference leaks, the pool slot is never released, and g_cleanup_inflight never returns to zero, so every later UbrCleanupWorker::DrainAndWait (DestroyShmTimer on a teardown path) hangs forever. Take a small mutex in Post() around the unavailable check, the start attempt and the enqueue, so a job can only ever be queued behind a worker that really started. Posting only happens on cleanup paths, the worker never takes the mutex, and a failed start now returns false (callers keep running the work inline) without having enqueued anything.
The UbrCleanupClaim members were the only constants in ub_ring_manager.h without the module's UBR_ prefix, next to UBR_MGR_UNIT_* and UBR_CLEANUP_*. Rename them to UBR_CLEANUP_CLAIM_NOT_OURS / _HAS_CTL / _OWNED_NULL.
…ract The Timer Management sections still implied all cleanup work runs on the timer thread. Document that the delayed-clear and pending-unmap callbacks only arbitrate ownership and post a job to the cleanup worker, and that a one-shot UbrTimerDel returning 0 now also guarantees the facade is done with the storage the handle slot lives in, so the caller may free the object holding it (UbrCleanupCtl on the delayed-clear path). Also refresh the ordering comment above UbrTrxCloseCallback: with the cleanup body on the worker, the guarantee rests on the generation gate the worker re-applies under the manager lock (IsUbrTrxSlotUsed) plus the per-trx UbrStopTrxTimer/UbrTimerDelAndWait waits, not on the timer thread serializing the cleanup itself.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.