[C++][Acero] Fix BackpressureConcurrentQueue::Pop() when empty #45421
Conversation
|
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format? or See also: |
| @@ -35,8 +35,7 @@ class ConcurrentQueue { | |||
| // | |||
| T Pop() { | |||
| std::unique_lock<std::mutex> lock(mutex_); | |||
| cond_.wait(lock, [&] { return !queue_.empty(); }); | |||
| return PopUnlocked(); | |||
| return PopUnlocked(lock); | |||
There was a problem hiding this comment.
The comment for Pop says it must be called on a non-empty queue?
There was a problem hiding this comment.
Hmm. Didn't See that one. But still i think it is just wrong comment. Original ConcurrentQueue::Pop does check for emptiness despite the comment. Original BackpressureConcurrentQueue:Popdoes not check for emptiness and there is no comment.
There was a problem hiding this comment.
Hmm. Do you have really meet some problem here?
There was a problem hiding this comment.
I encountered a problem when I reused BackpressureConcurrentQueue in new acero node (will PR soon) and pop on empty obviously caused segfaut. That was when I switched from ConcurrentQueue to BackpressureConcurrentQueue - was not expecting that. I think to the very least to comment should be moved form ConcurrentQueue::Pop (misleading) to BackpressureConcurrentQueue:Pop (missing), although I think unifying safe version of Pop makes much more sense. More verbose method like PopUnsafe can be added also if any regression occurs. LMK the way forward and I will update PR.
Rationale for this change
BackpressureConcurrentQueue::Pop() does not check for empty. This can lead to UB.
Are there any user-facing changes?
No