Replace custom timer with bthread timer and improve lifecycle safety - #3509
Open
zchuango wants to merge 2 commits into
Open
Replace custom timer with bthread timer and improve lifecycle safety#3509zchuango wants to merge 2 commits into
zchuango wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
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
UbrCleanupCtlanchored inUBRingManager, 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. |
| src/brpc/ubshm/ub_ring.cpp | Implements bthread-timer scheduling, delayed cleanup ownership, close-check backoff, and atomic I/O sequencing. |
| src/brpc/ubshm/ub_ring_manager.h | Adds per-slot generation + cleanup-ctl anchoring APIs to coordinate delayed cleanup across slot reuse. |
| src/brpc/ubshm/ub_ring_manager.cpp | Implements generation-safe release and teardown-time cancellation/waiting for in-flight cleanups. |
| src/brpc/ubshm/ub_helper.cpp | Removes eager timer module init (timers become lazy via bthread facade). |
| src/brpc/ubshm/ub_endpoint.cpp | Fixes poller SID updates by replacing existing set entries on ADD/MOD. |
| src/brpc/ubshm/timer/timer_mgr.h | Defines the bthread-timer facade API and documents callback/teardown expectations. |
| src/brpc/ubshm/timer/timer_mgr.cpp | Replaces timerfd/epoll/kqueue implementation with bthread timer task management and delete/wait semantics. |
| src/brpc/ubshm/shm/shm_ubs.cpp | Moves UBS cleanup work outside locks, switches cleanup timer to bthread, and fixes teardown ordering vs SDK finalize. |
| src/brpc/ubshm/common/common.h | Adds SEC_TO_USEC for consistent time conversions used by the new timer paths. |
| docs/en/ubring.md | Updates timer documentation to reflect bthread timer usage and backoff policy. |
| docs/cn/ubring.md | Same as English docs: updates timer documentation to reflect new bthread timer approach. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+233
to
241
| bthread_timer_t id = task->id.load(); | ||
| if (id != 0 && bthread_timer_del(id) == 0) { | ||
| ReleaseRef(task); // schedule: cancelled before dispatch | ||
| } // ==1: dispatched, OnFire (owned==false) | ||
| // releases it | ||
| ReleaseRef(task); // owner | ||
| return 0; // the callback is guaranteed never to run: the caller | ||
| // consumes the timer/callback reference | ||
| } |
Comment on lines
+48
to
+57
| // Non-blocking delete, safe from inside the timer callback itself. Does | ||
| // not wait for a running callback and does not protect `arg' on its own. | ||
| // Returns 0 when the call won the slot competition: a one-shot callback | ||
| // is guaranteed never to run, and the caller consumes any per-task | ||
| // resources it tracks for this timer (ownership of them transfers to the | ||
| // caller); for a periodic timer an already-started callback is not | ||
| // interrupted. Returns 1 when the callback has been dispatched (it | ||
| // consumes those resources itself on every exit) or its fate is still | ||
| // being settled by the scheduler -- the caller must not consume anything | ||
| // then. |
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
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.