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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
