[GitHub] [arrow] lidavidm commented on a diff in pull request #13191: ARROW-16592: [C++][Python][FlightRPC] Finish after failed writes

[GitBox <gi...@apache.org>]

lidavidm commented on code in PR #13191:
URL: https://github.com/apache/arrow/pull/13191#discussion_r878059568


##########
cpp/src/arrow/flight/transport/grpc/grpc_client.cc:
##########
@@ -350,16 +347,7 @@ class WritableDataStream : public FinishableDataStream<Stream, ReadPayload> {
   Status DoFinish() override {
     // This may be used concurrently by reader/writer side of a
     // stream, so it needs to be protected.
-    std::lock_guard<std::mutex> guard(finish_mutex_);
-
-    // Now that we're shared between a reader and writer, we need to
-    // protect ourselves from being called while there's an
-    // outstanding read.
-    std::unique_lock<std::mutex> read_guard(read_mutex_, std::try_to_lock);
-    if (!read_guard.owns_lock()) {
-      return MakeFlightError(FlightStatusCode::Internal,
-                             "Cannot close stream with pending read operation.");
-    }
+    std::lock_guard<std::mutex> guard(read_mutex_);

Review Comment:
   Basically, we have three choices of behavior when gRPC returns `false` on a write (write failed, but no error info):
   
   1. Swallow the error and continue on until the user eventually calls Finish to get the error. (Arrow <= 7)
   2. Raise a generic error to interrupt the writer, the user must catch this and still call Finish (Arrow 8)
   3. Call Finish for the user and raise the actual error, _presumably_ this will complete instantly as the reason why `false` was returned means that there is already an error (this PR)
   
   `read_mutex` is for any read operation which includes `Finish` (this is implicitly a read)



-- 
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