Fix stale AgentGroup TLS access after bthread migration - #3522
Open
bobhan1 wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The fix targets a bthread migration/TLS-staleness scenario but there’s no accompanying regression test exercising bthread suspend/resume migration behavior to prevent future regressions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes a bthread-migration correctness bug in bvar::detail::AgentGroup where Clang may retain a stale TLS reference across a suspend/resume on a different pthread, potentially causing cross-pthread agent-vector access and crashes.
Changes:
- Switch
_s_tls_blocksto the existing volatile-TLS accessor pattern (STATIC_MEMBER_BAIDU_VOLATILE_THREAD_LOCAL,BAIDU_GET/SET_VOLATILE_THREAD_LOCAL). - In each AgentGroup operation, resolve the current pthread’s TLS block-vector once and consistently use that local pointer.
- Route TLS initialization/cleanup writes through the existing setter accessor.
File summaries
| File | Description |
|---|---|
| src/bvar/detail/agent_group.h | Reworks _s_tls_blocks access to use volatile-TLS accessors and per-operation resolution to avoid stale TLS across bthread migration. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+116
to
+120
| std::vector<ThreadBlock*>* tls_blocks = | ||
| BAIDU_GET_VOLATILE_THREAD_LOCAL(_s_tls_blocks); | ||
| if (tls_blocks) { | ||
| const size_t block_id = (size_t)id / ELEMENTS_PER_BLOCK; | ||
| if (block_id < _s_tls_blocks->size()) { | ||
| ThreadBlock* const tb = (*_s_tls_blocks)[block_id]; | ||
| if (block_id < tls_blocks->size()) { |
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: resolve #3520
Problem Summary:
bvar::detail::AgentGroupdirectly accesses the raw_s_tls_blocksthread-local variable from inline functions. When a bthread suspends and resumes on another pthread, Clang may reuse the TLS address resolved for the previous pthread. This can make bvar access another pthread's agent vector and race with its initialization or resize.What is changed and the side effects?
Changed:
_s_tls_blockswith the existing static-member volatile TLS accessor mechanism.Side effects:
Check List: