[GitHub] [arrow] bkietz commented on a diff in pull request #37839: GH-37796: [C++][Acero] Fix race condition caused by straggling input in the as-of-join node

["bkietz (via GitHub)" <gi...@apache.org>]

bkietz commented on code in PR #37839:
URL: https://github.com/apache/arrow/pull/37839#discussion_r1339193790


##########
cpp/src/arrow/acero/asof_join_node.cc:
##########
@@ -1323,6 +1342,12 @@ class AsofJoinNode : public ExecNode {
           if (st.ok()) {
             st = output_->InputFinished(this, batches_produced_);
           }
+          for (const auto& s : state_) {
+            auto shutdownResult = s->ForceShutdown();
+            if (!shutdownResult.ok()) {
+              st = shutdownResult;
+            }

Review Comment:
   ```suggestion
               st &= s->ForceShutdown();
   ```



##########
cpp/src/arrow/acero/asof_join_node.cc:
##########
@@ -1679,6 +1704,15 @@ class AsofJoinNode : public ExecNode {
   const Ordering& ordering() const override { return ordering_; }
 
   Status InputReceived(ExecNode* input, ExecBatch batch) override {
+    // InputReceived may be called after execution was finished. Pushing it to the
+    // InputState may cause the BackPressureController to pause the input, causing a
+    // deadlock

Review Comment:
   ```suggestion
       // InputReceived may be called after execution was finished. Pushing it to the
       // InputState is unnecessary since we're done (and anyway may cause the
       // BackPressureController to pause the input, causing a deadlock), so drop it.
   ```
   
   Do we still deadlock *with* this short circuit but *without* ForceShutdown etc?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org