GH-45371: [C++] Fix data race in SimpleRecordBatch::columns
#45372
+26
−2
Conversation
This file contains 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
|
|
mapleFU
reviewed
Jan 29, 2025
mapleFU
reviewed
Jan 29, 2025
cpp/src/arrow/record_batch_test.cc
Outdated
| auto schema = ::arrow::schema({field("f1", utf8())}); | ||
| auto record_batch = RecordBatch::Make(schema, length, {array_data}); | ||
| std::atomic_bool start_flag{false}; | ||
| std::thread t([record_batch, &start_flag]() { |
There was a problem hiding this comment.
should more than one thread be tested here?
There was a problem hiding this comment.
Yes, this should use several threads that would do the same thing concurrently.
There was a problem hiding this comment.
In the current test the race is between t and the main thread. Only 2 threads are necessary to produce the data race.
pitrou
requested changes
Jan 29, 2025
cpp/src/arrow/record_batch_test.cc
Outdated
| auto schema = ::arrow::schema({field("f1", utf8())}); | ||
| auto record_batch = RecordBatch::Make(schema, length, {array_data}); | ||
| std::atomic_bool start_flag{false}; | ||
| std::thread t([record_batch, &start_flag]() { |
There was a problem hiding this comment.
Yes, this should use several threads that would do the same thing concurrently.
| std::thread t([record_batch, &start_flag]() { | ||
| start_flag.store(true); | ||
| auto columns = record_batch->columns(); | ||
| ASSERT_EQ(columns.size(), 1); |
There was a problem hiding this comment.
Why this test? boxed_columns_ is presized in the constructor, so this should always succeed.
There was a problem hiding this comment.
Yes, this assertion should always pass. The purpose is to ensure that columns is not optimized out. This test will either produce a TSAN warning or not, but there is no assertion that will fail consistently under the data race.
|
|
||
| random::RandomArrayGenerator gen(42); | ||
| std::shared_ptr<ArrayData> array_data = gen.ArrayOf(utf8(), length)->data(); | ||
| auto schema = ::arrow::schema({field("f1", utf8())}); |
There was a problem hiding this comment.
There could also be several fields and the worker thread would call column(i) several times with i being a random number. Something like (untested):
constexpr int kNumFields = 40;
constexpr int kNumThreads = 50;
auto schema = ::arrow::schema(FieldVector(kNumFields, field("f1", utf8())));
auto batch = RecordBatch::Make(schema, length, ArrayDataVector(kNumFields, array_data));
std::random_device rd;
std::vector<std::threads> threads(kNumThreads);
for (auto& thread : threads) {
const auto seed = rd();
thread = std::thread([&]() {
std::default_engine rng(seed);
std::uniform_int_distribution<int> field_dist(0, kNumFields - 1);
for (int i = 0; i < kNumFields; ++i) {
ASSERT_NE(nullptr, batch->column(field_dist(rng)));
}
});
}
for (auto& thread : threads) {
thread.join();
}There was a problem hiding this comment.
The data race only appears when columns() is called because it allows non-atomic reads to boxed_columns_. The column(i) function alone is thread safe because it only uses atomics. It will also never return nullptr.
added 3 commits
January 29, 2025 19:55
colin-r-schultz
force-pushed
the
recordbatch-columns-thread-safety
branch
from
87216a6 to
41e23d7
Compare
|
Thanks for taking the time to review my PR! Let me just clarify the intent behind my test case. It's a minimal example that produces a data race detected by TSAN. The data race only needs to occur between 2 threads, in this case I use the main thread and
Now, we simultaneously have T1 reading There isn't an assertion that will catch this case because it is impossible for Test outputRunning the test again with the proposed fix shows no data race. The Not sure if it makes sense to have a test that only works under TSAN, but I don't think there is any way to surface the bug consistently without tooling. |
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
GH-45371
What changes are included in this PR?
Use
std::atomic_compare_exchangeto initializeboxed_columns_[i]so they are correctly written only once. This means that a reference toboxed_columns_is safe to read after each element has been initialized.Are these changes tested?
Yes, there is a test case
TestRecordBatch.ColumnsThreadSafetywhich passes under TSAN.Are there any user-facing changes?
No
This PR contains a "Critical Fix".
Without this fix, concurrent calls to
SimpleRecordBatch::columnscould lead to an invalid memory access and crash.SimpleRecordBatch::columns#45371