You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "kou (via GitHub)" <gi...@apache.org> on 2023/06/29 05:50:03 UTC
[GitHub] [arrow] kou opened a new pull request, #36372: GH-36369: [C++][FlightRPC] Fix a hang bug in FlightClient::Authenticate*()
kou opened a new pull request, #36372:
URL: https://github.com/apache/arrow/pull/36372
### Rationale for this change
We need to drain all responses from server before we call gRPC's Finish() to avoid hanging.
### What changes are included in this PR?
Read all responses before calling Finish().
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Yes.
--
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
[GitHub] [arrow] pitrou commented on a diff in pull request #36372: GH-36369: [C++][FlightRPC] Fix a hang bug in FlightClient::Authenticate*()
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #36372:
URL: https://github.com/apache/arrow/pull/36372#discussion_r1246818195
##########
cpp/src/arrow/flight/transport/grpc/grpc_client.cc:
##########
@@ -740,11 +740,18 @@ class GrpcClientImpl : public internal::ClientTransport {
RETURN_NOT_OK(auth_handler_->Authenticate(&outgoing, &incoming));
// Explicitly close our side of the connection
bool finished_writes = stream->WritesDone();
- RETURN_NOT_OK(FromGrpcStatus(stream->Finish(), &rpc.context));
if (!finished_writes) {
return MakeFlightError(FlightStatusCode::Internal,
"Could not finish writing before closing");
}
+ // Drain the read side, as otherwise gRPC Finish() will hang. We
+ // only call Finish() when the client closes the writer or the
+ // reader finishes, so it's OK to assume the client no longer
+ // wants to read and drain the read side.
+ pb::HandshakeResponse response;
+ while (stream->Read(&response)) {
+ }
+ RETURN_NOT_OK(FromGrpcStatus(stream->Finish(), &rpc.context));
Review Comment:
Can we perhaps factor out all this in a helper method?
(also, why do we ignore any read response?)
--
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
[GitHub] [arrow] kou commented on pull request #36372: GH-36369: [C++][FlightRPC] Fix a hang bug in FlightClient::Authenticate*()
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #36372:
URL: https://github.com/apache/arrow/pull/36372#issuecomment-1616116320
Green.
I'll merge this.
--
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
[GitHub] [arrow] lidavidm commented on a diff in pull request #36372: GH-36369: [C++][FlightRPC] Fix a hang bug in FlightClient::Authenticate*()
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #36372:
URL: https://github.com/apache/arrow/pull/36372#discussion_r1246530580
##########
cpp/src/arrow/flight/transport/grpc/grpc_client.cc:
##########
@@ -740,11 +740,24 @@ class GrpcClientImpl : public internal::ClientTransport {
RETURN_NOT_OK(auth_handler_->Authenticate(&outgoing, &incoming));
// Explicitly close our side of the connection
bool finished_writes = stream->WritesDone();
- RETURN_NOT_OK(FromGrpcStatus(stream->Finish(), &rpc.context));
if (!finished_writes) {
return MakeFlightError(FlightStatusCode::Internal,
"Could not finish writing before closing");
}
+ // Drain the read side, as otherwise gRPC Finish() will hang. We
+ // only call Finish() when the client closes the writer or the
+ // reader finishes, so it's OK to assume the client no longer
+ // wants to read and drain the read side.
+ pb::HandshakeResponse response;
+ if (!stream->Read(&response)) {
+ return MakeFlightError(FlightStatusCode::Internal,
+ "No handshake response from server");
Review Comment:
Do we expect another response? I think the auth_handler_ should've read everything.
This could probably just be
```cpp
while (stream->Read(&response)) {}
```
here and below.
I think it's OK to either ignore all extra responses, or return an error.
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #36372: GH-36369: [C++][FlightRPC] Fix a hang bug in FlightClient::Authenticate*()
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36372:
URL: https://github.com/apache/arrow/pull/36372#issuecomment-1612466836
:warning: GitHub issue #36369 **has been automatically assigned in GitHub** to PR creator.
--
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
[GitHub] [arrow] thisisnic commented on pull request #36372: GH-36369: [C++][FlightRPC] Fix a hang bug in FlightClient::Authenticate*()
Posted by "thisisnic (via GitHub)" <gi...@apache.org>.
thisisnic commented on PR #36372:
URL: https://github.com/apache/arrow/pull/36372#issuecomment-1614459614
The remaining R failures will be fixed when #36397 is merged
--
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
[GitHub] [arrow] conbench-apache-arrow[bot] commented on pull request #36372: GH-36369: [C++][FlightRPC] Fix a hang bug in FlightClient::Authenticate*()
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #36372:
URL: https://github.com/apache/arrow/pull/36372#issuecomment-1623940280
Conbench analyzed the 6 benchmark runs on commit `f03943cf`.
There were 3 benchmark results indicating a performance regression:
- Commit Run on `arm64-t4g-linux-compute` at [2023-07-01 22:59:20Z](http://conbench.ursa.dev/compare/runs/7129dfd9d7034e6ead9b6f8ef150c896...9c2e2db84bd14ce99cb1638adb926fdf/)
- [params=num_cols:64/is_partial:1/real_time, source=cpp-micro, suite=arrow-ipc-read-write-benchmark](http://conbench.ursa.dev/compare/benchmarks/0649eea87c9d748d80002d0f6d3c8eff...064a0b028af977a38000426f297a3e0d)
- Commit Run on `ursa-thinkcentre-m75q` at [2023-07-04 13:05:02Z](http://conbench.ursa.dev/compare/runs/2eccc290ec124e7a8068a0fa6f40d016...4264a6e39e4344deb67edad8363f031e/)
- [params=<STATIC_VECTOR(std::shared_ptr<int>)>, source=cpp-micro, suite=arrow-small-vector-benchmark](http://conbench.ursa.dev/compare/benchmarks/064a3ed4e28e716f8000b5753edfed61...064a419529aa7490800048a81873b6bc)
- and 1 more (see the report linked below)
The [full Conbench report](https://github.com/apache/arrow/runs/14833215424) has more details.
--
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
[GitHub] [arrow] kou commented on a diff in pull request #36372: GH-36369: [C++][FlightRPC] Fix a hang bug in FlightClient::Authenticate*()
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #36372:
URL: https://github.com/apache/arrow/pull/36372#discussion_r1246673515
##########
cpp/src/arrow/flight/transport/grpc/grpc_client.cc:
##########
@@ -740,11 +740,24 @@ class GrpcClientImpl : public internal::ClientTransport {
RETURN_NOT_OK(auth_handler_->Authenticate(&outgoing, &incoming));
// Explicitly close our side of the connection
bool finished_writes = stream->WritesDone();
- RETURN_NOT_OK(FromGrpcStatus(stream->Finish(), &rpc.context));
if (!finished_writes) {
return MakeFlightError(FlightStatusCode::Internal,
"Could not finish writing before closing");
}
+ // Drain the read side, as otherwise gRPC Finish() will hang. We
+ // only call Finish() when the client closes the writer or the
+ // reader finishes, so it's OK to assume the client no longer
+ // wants to read and drain the read side.
+ pb::HandshakeResponse response;
+ if (!stream->Read(&response)) {
+ return MakeFlightError(FlightStatusCode::Internal,
+ "No handshake response from server");
Review Comment:
OK. I've simplified this.
--
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
[GitHub] [arrow] kou merged pull request #36372: GH-36369: [C++][FlightRPC] Fix a hang bug in FlightClient::Authenticate*()
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou merged PR #36372:
URL: https://github.com/apache/arrow/pull/36372
--
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
[GitHub] [arrow] kou commented on a diff in pull request #36372: GH-36369: [C++][FlightRPC] Fix a hang bug in FlightClient::Authenticate*()
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #36372:
URL: https://github.com/apache/arrow/pull/36372#discussion_r1247309683
##########
cpp/src/arrow/flight/transport/grpc/grpc_client.cc:
##########
@@ -740,11 +740,18 @@ class GrpcClientImpl : public internal::ClientTransport {
RETURN_NOT_OK(auth_handler_->Authenticate(&outgoing, &incoming));
// Explicitly close our side of the connection
bool finished_writes = stream->WritesDone();
- RETURN_NOT_OK(FromGrpcStatus(stream->Finish(), &rpc.context));
if (!finished_writes) {
return MakeFlightError(FlightStatusCode::Internal,
"Could not finish writing before closing");
}
+ // Drain the read side, as otherwise gRPC Finish() will hang. We
+ // only call Finish() when the client closes the writer or the
+ // reader finishes, so it's OK to assume the client no longer
+ // wants to read and drain the read side.
+ pb::HandshakeResponse response;
+ while (stream->Read(&response)) {
+ }
+ RETURN_NOT_OK(FromGrpcStatus(stream->Finish(), &rpc.context));
Review Comment:
> Can we perhaps factor out all this in a helper method?
Done.
> (also, why do we ignore any read response?)
For `Authenticate()`, user defined `auth_handler_->Authenticate()` may or may not read a response from a server. If `auth_handler_->Authenticate()` isn't read a response, we can read it here. But we can't do nothing here for the read response. So we can just ignore the read response.
For `AuthenticateBasicToken()`, we can read one response from a server here. But we don't need to use the response for basic token authentication. So we can just ignore the read response.
A server may return multiple responses but we don't need them here. So we can ignore them too.
--
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