GH-45953: [C++] Use lock to fix atomic bug in ReadaheadGenerator #45954
Conversation
|
|
|
|
|
sidenote: a |
|
@mapleFU I can confirm this fixes the Only the |
|
@pitrou @zanmato1984 This seems fixed the bug, however, lint disables |
You can use |
|
FWIW, it is because of C++/CLI: #45810 |
A related discussion about the lint rule and the deprecation of support for C++/CLI can be found here #45674 (comment). And it turns out that we are good to get rid of it. |
Right, this works too. |
|
|
mapleFU
changed the title
|
|
|
@pitrou @zanmato1984 would you mind take a look? |
cpp/src/arrow/util/async_generator.h
Outdated
| std::atomic<bool> finished{false}; | ||
| int num_running{0}; // GUARDED_BY(mu) | ||
| bool finished{false}; // GUARDED_BY(mu) | ||
| arrow::util::Mutex mu; |
cpp/src/arrow/util/async_generator.h
Outdated
| Future<> final_future = Future<>::Make(); | ||
| std::atomic<int> num_running{0}; | ||
| std::atomic<bool> finished{false}; | ||
| int num_running{0}; // GUARDED_BY(mu) |
There was a problem hiding this comment.
Is GUARDED_BY an annotation for some kind of static analysis tool? Which one?
There was a problem hiding this comment.
This is just my code style, also related tool is https://clang.llvm.org/docs/ThreadSafetyAnalysis.html , I just follow same style here
github-actions
bot
added
awaiting committer review
|
@github-actions crossbow submit -g cpp |
|
Revision: 740b1cf Submitted crossbow builds: ursacomputing/crossbow @ actions-c21700c42d
|
zanmato1984
left a comment
zanmato1984
left a comment
There was a problem hiding this comment.
Very nice fix.
LGTM with one minor suggestion.
| if (!is_finished) { | ||
| ++state_->num_running; | ||
| } |
There was a problem hiding this comment.
Suggestion: add a comment to explain this
| if (!is_finished) { | |
| ++state_->num_running; | |
| } | |
| if (!is_finished) { | |
| // We're going to push to the queue below, but we need | |
| // to update `num_running` while we're holding the lock. | |
| ++state_->num_running; | |
| } |
pitrou
left a comment
pitrou
left a comment
There was a problem hiding this comment.
Just a suggestion, otherwise LGTM
|
Comment fixed, I'll merge this after all ci passes |
|
After merging your PR, Conbench analyzed the 0 benchmarking runs that have been run so far on merge-commit 19033bc. None of the specified runs were found on the Conbench server. The full Conbench report has more details. |
apache#45954) ### Rationale for this change The lambda continuation in `ReadaheadGenerator::AddMarkFinishedContinuation` might have a bug in below ordering: ``` Thread1: check state_->finished.load() == true in 1 Thread2: state->MarkFinishedIfDone, state->final_future.MarkFinished Thread1: state_->num_running.fetch_add(1) in 2 Thread1: state->final_future.MarkFinished ``` Which thread1 is a thread calling `ReadaheadGenerator::operator()`, thread2 is triggered by underlying async generator. This is because `finished` and `num_running` are different atomic variables. ### What changes are included in this PR? using lock instead ### Are these changes tested? Covered by existing ### Are there any user-facing changes? Bugfix * GitHub Issue: apache#45953 Authored-by: mwish <[email protected]> Signed-off-by: mwish <[email protected]>
4 participants
Rationale for this change
The lambda continuation in
ReadaheadGenerator::AddMarkFinishedContinuationmight have a bug in below ordering:Which thread1 is a thread calling
ReadaheadGenerator::operator(), thread2 is triggered by underlying async generator.This is because
finishedandnum_runningare different atomic variables.What changes are included in this PR?
using lock instead
Are these changes tested?
Covered by existing
Are there any user-facing changes?
Bugfix