Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Comments

Fix data race in MariaModule groups_ vector access#210

Draft
Copilot wants to merge 3 commits intoeloq-10.6.10 from
copilot/fix-marimodule-concurrent-issue
Draft

Fix data race in MariaModule groups_ vector access #210
Copilot wants to merge 3 commits intoeloq-10.6.10 from
copilot/fix-marimodule-concurrent-issue

Conversation

Copy link

Copilot AI commented Jan 16, 2026
edited
Loading

The groups_ vector in MariaModule was accessed concurrently by external worker threads (in Process(), HasTask()) and SQL threads (in ExtThdStart(), ExtThdEnd()) without synchronization. Concurrent writes via ResizeGroups() and SetGroup() during pool size changes caused vector reallocation races and nullptr dereferences.

Changes

  • Added mutable std::mutex groups_mutex_ to MariaModule class
  • Protected writers: ResizeGroups() and SetGroup() now acquire mutex during vector modification
  • Protected readers: ExtThdStart(), ExtThdEnd(), Process(), and HasTask() now:
    • Acquire mutex only while reading from groups_ vector
    • Copy pointer to local variable before releasing lock
    • Check for nullptr before dereferencing

Pattern

All reader methods now follow:

thread_group_t *group = nullptr;
{
 std::lock_guard<std::mutex> lk(groups_mutex_);
 if (thd_id >= 0 && (size_t)thd_id < groups_.size()) {
 group = groups_[thd_id];
 }
}
if (group == nullptr || group->shutdown.load(std::memory_order_relaxed))
 return;
 
// Safe to use group pointer outside lock
group->ext_worker_active_.store(true, std::memory_order_relaxed);

This ensures no data races during vector reallocation while minimizing lock contention by releasing the mutex before accessing thread_group_t members.

Original prompt

Fix a concurrent issue in MariaModule in sql/threadpool_generic.cc and sql/threadpool_generic.h.

Background:
The MariaModule is registered as a module of brpc. The groups_ vector in MariaModule stores pointers to thread_group_t. These pointers are accessed by external worker threads in Process() and HasTask(), and by SQL threads in ExtThdStart() and ExtThdEnd().
Concurrently, the groups_ vector is modified in ResizeGroups() and SetGroup() when the thread pool size changes (e.g., via TP_pool_generic::set_pool_size).

Issue:
There is no synchronization between threads reading from groups_ and threads writing to it.
Specifically:

  1. ResizeGroups() calls groups_.resize(size, nullptr), which fills new slots with nullptr and may reallocate the vector's internal storage, causing data races with concurrent readers.
  2. Process(), HasTask(), ExtThdStart(), and ExtThdEnd() do not check if the retrieved thread_group_t* is nullptr before dereferencing it (e.g., group->shutdown.load() or group->ext_worker_active_.store()).
  3. GDB reports that group can be 0x0 even if groups_[thd_id] appears non-zero later, indicating a memory visibility/concurrency issue.

Requested Fix:

  1. In sql/threadpool_generic.h, add a mutable std::mutex groups_mutex_; to the MariaModule class.
  2. In sql/threadpool_generic.h, ensure #include <mutex> is available for the MariaModule class (move it out of #ifdef COROUTINE_ENABLED if necessary or add it to the ELOQ_MODULE_ENABLED block).
  3. In sql/threadpool_generic.cc, update ExtThdStart, ExtThdEnd, Process, and HasTask to:
    • Use std::lock_guard<std::mutex> with groups_mutex_ when accessing the groups_ vector to get the thread_group_t*.
    • Add a check to return immediately if the retrieved group pointer is nullptr.
  4. Update ResizeGroups and SetGroup (either in .h or .cc) to use groups_mutex_ to protect the vector modification.

Example of desired pattern in Process:

void MariaModule::Process(int thd_id)
{
 thread_group_t *group = nullptr;
 {
 std::lock_guard<std::mutex> lk(groups_mutex_);
 if (thd_id >= 0 && (size_t)thd_id < groups_.size()) {
 group = groups_[thd_id];
 }
 }
 if (group == nullptr || group->shutdown.load(std::memory_order_relaxed))
 {
 return;
 }
 // ... rest of the function
}

Apply similar patterns to ExtThdStart, ExtThdEnd, and HasTask. Note that HasTask is const, so the mutex must be mutable.

This pull request was created from Copilot chat.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copy link

coderabbitai bot commented Jan 16, 2026

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copilot AI and others added 2 commits January 16, 2026 06:55
- Added mutable std::mutex groups_mutex_ to MariaModule class
- Updated ResizeGroups() and SetGroup() to protect vector modification with mutex
- Updated ExtThdStart() to use mutex and check for nullptr before dereferencing
- Updated ExtThdEnd() to use mutex and check for nullptr before dereferencing
- Updated Process() to use mutex and check for nullptr before dereferencing
- Updated HasTask() to use mutex and check for nullptr before dereferencing
Co-authored-by: githubzilla <10080336+githubzilla@users.noreply.github.com>
Added defensive bounds checking in SetGroup() to prevent potential out-of-bounds access.
Fixed code style to use space around assignment operator.
Co-authored-by: githubzilla <10080336+githubzilla@users.noreply.github.com>
Copilot AI changed the title (削除) [WIP] Fix concurrent issue in MariaModule's groups vector (削除ここまで) (追記) Fix data race in MariaModule groups_ vector access (追記ここまで) Jan 16, 2026
Copilot AI requested a review from githubzilla January 16, 2026 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@githubzilla githubzilla Awaiting requested review from githubzilla

At least 1 approving review is required to merge this pull request.

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

AltStyle によって変換されたページ (->オリジナル) /