GH-50239: [R] Data race issue from R API requests in parallel region#50488
Open
thisisnic wants to merge 1 commit into
Open
GH-50239: [R] Data race issue from R API requests in parallel region#50488thisisnic wants to merge 1 commit into
thisisnic wants to merge 1 commit into
Conversation
|
|
Member
Author
|
@ursabot please benchmark |
Member
|
Benchmark runs are scheduled for commit 824f648. Watch https://buildkite.com/apache-arrow and https://conbench.arrow-dev.org for updates. A comment will be posted here when the runs are complete. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a ThreadSanitizer-detected data race in the R bindings when use_threads = TRUE by preventing worker threads from touching R vectors until after all R-side allocations and list assignments are complete on the main thread.
Changes:
- Delay scheduling of parallel
RTaskswork untilRTasks::Finish()instead of submitting immediately inRTasks::Append(). - Add storage for delayed parallel tasks and ensure it is cleared on
Reset().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| r/src/RTasks.cpp | Queues parallel tasks in Append(), then submits them in Finish() after serial work completes; clears the new queue in Reset(). |
| r/src/r_task_group.h | Adds delayed_parallel_tasks_ to hold parallel tasks until Finish(). |
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.
Rationale for this change
When
use_threads = TRUE, parallel tasks are submitted to the thread pool immediately duringRTasks::Append(). This means worker threads start reading R vector headers (viaINTEGER(),REAL(), etc.) while the main thread is still assigning vectors into the output list viaSET_VECTOR_ELT, which writes to the same header memory. ThreadSanitizer detects this as a data race.What changes are included in this PR?
Delay submission of parallel tasks until
RTasks::Finish(), matching the existing pattern for serial tasks. All R-side allocations and list assignments complete on the main thread before any worker threads start.Are these changes tested?
Existing tests cover the affected code path.
I also ran some local benchmarks to see if this change affected performance, but seems fine.
Are there any user-facing changes?
No.