Repository navigation
Conversation
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).
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.
Sherlock0203
added a commit
to Sherlock0203/brpc
that referenced
this pull request
Oct 16, 2026
Bring the negotiation and IPC_V2 work that landed on master (apache#3507, apache#3573) into this branch so it can be merged back without conflicts. Those master changes were built on the timerfd-era close path, which this branch had replaced with the bthread timer facade, generation-checked pool teardown and the cleanup worker, so both sides evolved the same ubshm core after 9334389. Resolution: - Close/clear core (UbrTrxClose, UbrTrxCloseCallback, UbrClearResourceCheck, ClearTrxResource, UbrPassiveClearTrx, UbrTrxCloseCheck) keeps this branch's versions. Master's extra start_time/op parameters were unused, and UbrCloseType only chose between DeleteTimerSafe and DeleteTimer, which the facade subsumes: UbrTimerDelAndWait degrades to the non-blocking delete inside a callback. The unused UbrCloseType enum is therefore dropped. - Master's format-aware initialization is kept: UbrServerTrxInit keeps the UbrDataFormat parameter and the Initialize*LegacyFormat split, UbrAllocateServerShm/UbrMapRemoteShm(UbrMapRemoteShmAddTimer) keep passing it, and ApplyAndMapLocalShm no longer presets the rx capacity (InitializeClientLocalLegacyFormat owns that now). All IPC_V2 ring code and the negotiation handshake are master's, unchanged. - Master-side UNLIKELY/LIKELY are converted to BAIDU_UNLIKELY/ BAIDU_LIKELY, matching the module convention from this branch's style pass; common.h keeps the plain aliases. - This branch's fixes that master lacked merge on top: the poller re-ADD refresh, butil::atomic for the poller-group flag, the lazy bthread timers replacing TimerInit, and the cleanup-worker changes (leaked queue, start retry instead of the terminal latch). - test/brpc_ubring_unittest.cpp: master's IPC_V2 assertions on the removed timer_fd/hb_timer_fd fields now check the unarmed close_timer/hb_timer slots instead. The UbrCleanupWorkerTest that PR LinQuickDev#74's squash dropped is restored (the commit describes it and the g_ub_test_worker_start_rc hook it exercises, but only the two source files made it into the squash), so both sides' suites run: 31 tests. Tested: cmake --build build --target brpc_ubring_unittest (WITH_UBRING =ON, BUILD_UNIT_TESTS=ON) and ./build/test/brpc_ubring_unittest (31/31), also under ASAN (detect_leaks=0, 31/31). e2e with example/ubring_performance --use_ubring=true over the IPC backend: UBRING_* mappings confirmed, the negotiated IPC_V2 data path sustains ~148k QPS at ~6us, phase teardowns are clean, and a server kill -9 is handled without errors or residue.
Sherlock0203
added a commit
to Sherlock0203/brpc
that referenced
this pull request
Oct 19, 2026
Bring the apache master line up to f0239d7 into this branch in a single merge, replacing the two stacked merges with master@36c2d12e and the macOS fix only. Everything the branch needs from that line is included: the data-format negotiation (apache#3507), the IPC_V2 data path (apache#3573), the macOS OpenSSL 4.0 build fix (apache#3616: X509 accessors return const pointers in the current macos-26-arm64 runner image, fixed with auto*), and the newer fixes, notably the null-termination of the peer shm_name in HelloMessage::Deserialize (apache#3605), the MPSC queue node release ordering (apache#3620) and the MultiFlatMap bucket-head erasure fix (apache#3617). Those master changes were built on the timerfd-era close path, which this branch had replaced with the bthread timer facade, generation-checked pool teardown and the cleanup worker, so both sides evolved the same ubshm core after 9334389. Resolution: - Close/clear core (UbrTrxClose, UbrTrxCloseCallback, UbrClearResourceCheck, ClearTrxResource, UbrPassiveClearTrx, UbrTrxCloseCheck) keeps this branch's versions. Master's extra start_time/op parameters were unused, and UbrCloseType only chose between DeleteTimerSafe and DeleteTimer, which the facade subsumes: UbrTimerDelAndWait degrades to the non-blocking delete inside a callback. The unused UbrCloseType enum is therefore dropped. - Master's format-aware initialization is kept: UbrServerTrxInit keeps the UbrDataFormat parameter and the Initialize*LegacyFormat split, UbrAllocateServerShm/UbrMapRemoteShm(UbrMapRemoteShmAddTimer) keep passing it, and ApplyAndMapLocalShm no longer presets the rx capacity (InitializeClientLocalLegacyFormat owns that now). All IPC_V2 ring code and the negotiation handshake are master's, unchanged, with master-side UNLIKELY/LIKELY converted to the module's BAIDU_* macros. - This branch's fixes that master lacked merge on top: the poller re-ADD refresh, butil::atomic for the poller-group flag, the lazy bthread timers replacing TimerInit, and the cleanup-worker changes (leaked queue, start retry instead of the terminal latch). - test/brpc_ubring_unittest.cpp: master's IPC_V2 assertions on the removed timer_fd/hb_timer_fd fields check the unarmed close_timer/hb_timer slots instead, and the UbrCleanupWorkerTest that PR LinQuickDev#74's squash dropped is restored alongside master's apache#3605 hardening of the full_shm_name test. Both sides' suites run. Tested: the close/cleanup/IPC_V2 resolution was verified on the equivalent tree before apache#3605 came in -- cmake WITH_UBRING=ON build, 31/31 unit tests, the same under ASAN (detect_leaks=0), and an example/ubring_performance e2e over the IPC backend (negotiated IPC_V2 path at ~148k QPS, clean teardowns, server kill -9 handled). The new master commits are additive to that resolution; cmake --build build --target brpc_ubring_unittest and ./build/test/brpc_ubring_unittest pass on this exact tree as well.
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.
No description provided.