You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Replace custom timer with bthread timer and improve lifecycle safety - #3509
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the UBRing/UBShmTransport timer and teardown machinery to use bthread timers, with additional lifecycle-safety mechanisms (generation checks + anchored cleanup control objects) to prevent stale timer/callback paths from touching reused trx slots and to avoid blocking work on the timer thread.
Changes:
Replace the custom timerfd/epoll timer subsystem with a bthread-timer-based facade (UbrTimerStart / UbrTimerDel / UbrTimerDelAndWait) and update call sites accordingly.
Introduce explicit delayed-cleanup ownership tracking via UbrCleanupCtl anchored in UBRingManager, plus generation checks to avoid slot-reuse races.
Reduce idle close-check polling via exponential backoff, and improve teardown ordering (e.g., stop UBS SHM cleanup timer before SDK finalize).
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
File
Description
test/brpc_ubring_unittest.cpp
Extends configuration tests to cover the new close-check backoff cap flag default.
src/brpc/ubshm/ubr_trx.h
Replaces fd-based timers with bthread timer handles; adds cleanup control state and makes hot counters atomic.
src/brpc/ubshm/ub_ring.h
Updates UBRing APIs to remove timer-fd parameters and align cleanup entry points with new lifecycle model.
)
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.
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).
Offload synchronous SDK cleanup from the bthread timer thread
src/brpc/ubshm/shm/shm_ubs.cpp:453
These synchronous UBS SDK calls still run inline on the process-wide bthread timer thread. ubsmem_shmem_unmap/ubsmem_shmem_deallocate can perform remote daemon/network work (the code explicitly handles UBSM_ERR_NET), so a slow cleanup will delay every UBRing heartbeat, close-check, and unrelated brpc timer. Dispatch the SDK cleanup to a worker/bthread and keep the timer callback limited to claiming/removing work; this is also the lightweight-callback requirement described in issue #3463.
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).
…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).
@chenBright@wwbmmm I’ve pushed follow-up fixes for the recent timer lifecycle and cleanup races. The latest head is 28f3470. Could you take another look and let me know if there are any remaining blockers? If there are none, I plan to merge #3509.
[replied by brpc-oncall robot] Changes needed: the lifecycle rework (task anchored across a self-deleting periodic callback, atomic ctl publication under the manager lock, ArmTimersExclusive/BeginPoolAccess shutdown barrier) resolves the previously raised races, but UBRing::UbrTrxClose now dereferences _trx before the existing null check, so a null _trx crashes instead of being rejected.
src/brpc/ubshm/ub_ring.cpp:199_trx is dereferenced here before UbrTrxCloseCheck runs, so the trx == nullptr guard inside it (and its "client trx is null" log) is now dead code and a null _trx becomes a null-pointer dereference. That case is reachable: _trx is initialized to nullptr and several setup failure paths reset it to nullptr (e.g. UbrAllocateLocalShm/UbrAllocateServerShm), while UBShmEndpoint::DeallocateResources calls _ub_ring->UbrTrxClose() unconditionally. Please restore the check before the load, e.g. return UBRING_ERR if _trx == nullptr and only then compute expect_ubr_id, or have UbrTrxCloseCheck take _trx and do the generation load after its own null check.
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).
@wwbmmm I added the null check before reading _trx->ubr_id in 6305d2d, and CI is green on the latest commit. Could you take another look when you have a chance?
The reason will be displayed to describe this comment to others. Learn more.
This schedules UbsShmCallback on the process-wide bthread timer thread, where it still directly calls ubsmem_shmem_unmap and ubsmem_shmem_deallocate.
Moving these calls outside shm_lock and processing one node per invocation reduces lock contention and batch size. A single slow SDK call can still occupy the global timer thread for its entire duration.
Previously, this work ran on a dedicated UBRing timer thread. With this change, a slow daemon or network failure can also delay ordinary RPC timeouts, backup requests, and bthread sleep/timed-wait wakeups across the process. The delayed transaction cleanup callbacks perform synchronous SDK cleanup on the same thread as well.
The reason will be displayed to describe this comment to others. Learn more.
UBS cleanup now runs on a worker, so the normal path no longer calls the SDK from the global timer thread. If the worker fails to start, it still falls back to inline cleanup.
The reason will be displayed to describe this comment to others. Learn more.
SnapshotUnitCleanupCtl releases the manager lock before this branch claims cleanup ownership. A concurrent SDK fault callback can publish a cleanup control object after the snapshot returned nullptr.
This is reachable while an active close is in progress: UbrPassiveClearTrx receives UBRING_REENTRY and proceeds through ClearTrxResource.
The following interleaving allows two cleanup owners:
Force-close stops the periodic timers and snapshots a null control object.
The SDK fault callback publishes a new control object for the same transaction generation and arms its cleanup timer.
Force-close sets cleanup_owned = true using its earlier null snapshot.
The new timer callback independently claims its control object and starts cleanup while the transaction slot is still marked USED.
With the explicitly supported ub_flying_io_timeout_s=0, or a sufficiently slow synchronous cleanup, both paths can unmap the same shared memory or access event queues after they have been unmapped.
The reason will be displayed to describe this comment to others. Learn more.
There is still a lifetime race when a one-shot timer has been dispatched but UbrTimerOnFire has not yet performed its handle-slot CAS:
UbrTimerDel takes ctl->timer.
The underlying bthread_timer_del returns 1, but UbrTimerDel returns 0 because it won the slot.
This path releases the timer/callback reference. Once only the manager anchor remains, UbrMgrFini can release that anchor and delete ctl.
The dispatched wrapper resumes and performs its CAS through task->slot, which points to the freed ctl->timer.
Winning the slot suppresses the user callback, but the wrapper still accesses the slot before returning. The remaining schedule reference protects UbrTimerTask; it does not keep UbrCleanupCtl alive. Force-close followed by slot reuse can expose the same race when the old manager anchor is retired.
The reason will be displayed to describe this comment to others. Learn more.
UbrTimerDel now waits for an already-dispatched one-shot timer wrapper to finish accessing the slot before returning, so the control object can be released safely.
* fix(ubshm): retire the slot storage before a deleted one-shot timer returns
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).
* fix(ubshm): claim forced cleanup and cleanup publication atomically
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.
* fix(ubshm): offload ubs sdk cleanup from the global timer thread
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.
* fix(ubshm): serialize cleanup worker posting with the lazy start
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.
* style(ubshm): prefix the cleanup claim results with UBR_CLEANUP_CLAIM
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.
* docs(ubshm): document the cleanup worker and the one-shot delete contract
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.
The reason will be displayed to describe this comment to others. Learn more.
The worker runs indefinitely and its thread ID is discarded, while g_cleanup_queue is a global object with a destructor.
If the process exits with cleanup jobs still queued, MPSCQueue::~MPSCQueue() calls DequeueImpl(nullptr) concurrently with the worker's Dequeue(). This creates two consumers of a queue whose _cur_dequeue_node is non-atomic, allowing both threads to select and free the same node.
The reason will be displayed to describe this comment to others. Learn more.
The worker addresses the timer-thread blocking issue on the normal path. However, a temporary failure of bthread_start_background(), such as ENOMEM, permanently sets g_worker_unavailable. All subsequent cleanup submissions then fall back to inline execution, even after resources recover.
Here, that executes synchronous ubsmem_shmem_unmap() and ubsmem_shmem_deallocate() calls on the process-wide TimerThread. The delayed transaction cleanup callbacks have the same fallback, so a slow SDK daemon can still delay unrelated RPC timeouts, backup requests, and bthread timer wakeups
The reason will be displayed to describe this comment to others. Learn more.
Worker startup is now retried on each post. A failed start falls back to inline cleanup for that post only; later posts can use the worker once startup succeeds.
* fix(ubshm): keep the cleanup queue alive past process exit
g_cleanup_queue was a file-scope MPSCQueue with a destructor, while the
cleanup worker is a bthread that is never stopped: the bthread runtime
itself is not destructed at exit (see the note in task_control.cpp), so
the worker can still be inside Dequeue() when static destructors run.
MPSCQueue's destructor drains the queue, turning that destruction into a
second concurrent consumer of a single-consumer queue: _cur_dequeue_node
is a plain pointer, so the destructor and the worker could pick and free
the same node (double free) or interleave their advances (lost nodes). A
timer callback firing during the destructor phase would additionally
enqueue into a destroyed queue.
No orderly teardown drains the queue at exit -- GlobalRelease only runs
on init failure and in UT teardown -- so the residue the destructor would
drain is really possible.
Allocate the queue with new and never delete it, following the brpc
convention for globals entangled with threads that never stop
(g_task_control itself); the kernel reclaims it at exit. DrainAndWait
observes g_cleanup_inflight, not the queue, so teardown semantics are
unchanged. g_cleanup_inflight (trivially destructible atomics) and
g_worker_mtx (a static initializer, nothing to destroy) keep their
storage.
Tested: cmake --build build --target brpc_ubring_unittest, and
./build/test/brpc_ubring_unittest (12/12 passed).
* fix(ubshm): retry the cleanup worker start instead of disabling it
A failed bthread_start_background in Post() latched g_worker_unavailable
forever, so every later cleanup fell back to running inline: the SDK
calls went back to the process-wide timer thread (UbsShmDrainNode from
UbsShmCallback, the delayed trx cleanups from their callbacks), delaying
unrelated RPC timeouts, backup requests and bthread wakeups -- exactly
what the worker exists to avoid. bthread creation fails transiently
(ENOMEM), so the degradation correlated with memory pressure, the moment
where blocking the timer thread hurts most, and it never self-healed.
Drop the terminal flag and retry the lazy start on every post made while
the worker is not started. A failed attempt leaves g_worker_started
false and fails only that post; the caller runs it inline as the safety
net, and the next post retries the start. The posting mutex already
serializes the start attempt with the enqueue, so a job is still only
ever queued behind a start that succeeded -- the flag was redundant
double insurance for orphaned jobs. The error is logged with
LOG_EVERY_SECOND so a failure window cannot spam.
Add the g_ub_test_worker_start_rc hook so the new UT can inject a start
failure deterministically (no real bthread is created while it is set),
and cover both review cases: repeated failures fail only their own post
and queue nothing, and the first post after the injection is cleared
retries the start, runs the job exactly once and lets DrainAndWait
observe quiescence. The suite must observe the worker in its unstarted
state, so it is declared ahead of UBShmEndpointTest, whose teardown
posts cleanups, and probes for an already-started worker to skip rather
than assert on a start that is never attempted.
Tested: cmake --build build --target brpc_ubring_unittest, and
./build/test/brpc_ubring_unittest (13/13 passed). Reintroducing the
latch locally makes the new UT fail, confirming it guards the retry.
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
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.
What problem does this PR solve?
Issue Number: #3463（Phase2）
Problem Summary:
This PR addresses the Phase 2 timer and lifecycle stability improvements for UBShmTransport/UBRing discussed in #3463.
The existing UBRing timer and teardown implementation has several lifecycle and scalability issues, including:
UbrTrxrelease and slot reuse;What is changed and the side effects?
Changed:
bthread_timer_add/bthread_timer_del.UbrTimerDelAndWaitfor teardown paths that need to wait for in-flight callbacks.UbrCleanupCtlto coordinate delayed cleanup and force-close ownership independently of pooledUbrTrxobjects.ub_flying_io_timeout_ssleep from the timer callback and use delayed cleanup instead.shm_lockand stop the cleanup timer beforeubsmem_finalize.Side effects:
Performance effects:
Breaking backward compatibility:
Check List:
brpc_ubring_unittestpasses.environment.