You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "indigophox (via GitHub)" <gi...@apache.org> on 2024/02/13 21:11:55 UTC
[PR] GH-39791: [C++][Flight RPC] Flight C++ ServerMiddleware::SendingHeaders call ordering fix [arrow]
indigophox opened a new pull request, #40071:
URL: https://github.com/apache/arrow/pull/40071
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how
to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). 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](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
-->
### Rationale for this change
Documentation for C++ Flight server middleware implies that SendingHeaders is called when headers are to be sent, however it is instead called when middleware is initialized, prior to the app's RPC handler. This is also inconsistent with e.g. Java where the call happens post-handler.
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
### What changes are included in this PR?
[In-progress] Fix for the above call ordering, so far affecting only non-streaming handlers, including DoAction() but not yet including Handshake(). This PR should also [pending] include a more involved fix for DoGet, DoPut, and DoExchange, for which helpers are already present.
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
### Are these changes tested?
[Pending further commits and finalization of the proposed patch.]
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
### Are there any user-facing changes?
Barely: There is a "breaking" change in that if a C++ RPC handler mutates the call's middleware instance in a way that affects its SendingHeaders() behaviour, this will now work as expected. However, apps relying on this being broken (i.e. doing something currently moot and expecting it not to work) are themselves fundamentally broken so I am not concerned about this breaking aspect of this change.
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please uncomment the line below and explain which changes are breaking.
-->
**This PR includes breaking changes to public APIs.**
See above regarding user-facing changes. This SHOULD have zero affect on even remotely sane applications, as above.
<!--
Please uncomment the line below (and provide explanation) if the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld). We use this to highlight fixes to issues that may affect users without their knowledge. For this reason, fixing bugs that cause errors don't count, since those are usually obvious.
-->
<!-- **This PR contains a "Critical Fix".** -->
--
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
Re: [PR] GH-39791: [C++][Flight RPC] Flight C++ ServerMiddleware::SendingHeaders call ordering fix [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #40071:
URL: https://github.com/apache/arrow/pull/40071#issuecomment-1942550125
:warning: GitHub issue #39791 **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
Re: [PR] GH-39791: [C++][Flight RPC] Flight C++ ServerMiddleware::SendingHeaders call ordering fix [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #40071:
URL: https://github.com/apache/arrow/pull/40071#discussion_r1491824731
##########
cpp/src/arrow/flight/transport/grpc/grpc_server.cc:
##########
@@ -336,21 +406,35 @@ class GrpcServiceHandler final : public FlightService::Service {
if (instance != nullptr) {
flight_context.middleware_.push_back(instance);
flight_context.middleware_map_.insert({factory.first, instance});
- instance->SendingHeaders(&outgoing_headers);
}
}
+ // TODO factor this out after fixing all streaming and non-streaming handlers
+ if (!skip_headers) {
+ addMiddlewareHeaders(context, flight_context);
+ }
+
return ::grpc::Status::OK;
}
+ void addMiddlewareHeaders(ServerContext* context,
Review Comment:
it might be simpler to just have this be idempotent and then you can just sprinkle calls to it everywhere where we return to gRPC, and call it explicitly in a few places?
--
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
Re: [PR] GH-39791: [C++][Flight RPC] Flight C++ ServerMiddleware::SendingHeaders call ordering fix [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #40071:
URL: https://github.com/apache/arrow/pull/40071#discussion_r1492425297
##########
cpp/src/arrow/flight/transport/grpc/grpc_server.cc:
##########
@@ -336,21 +406,35 @@ class GrpcServiceHandler final : public FlightService::Service {
if (instance != nullptr) {
flight_context.middleware_.push_back(instance);
flight_context.middleware_map_.insert({factory.first, instance});
- instance->SendingHeaders(&outgoing_headers);
}
}
+ // TODO factor this out after fixing all streaming and non-streaming handlers
+ if (!skip_headers) {
+ addMiddlewareHeaders(context, flight_context);
+ }
+
return ::grpc::Status::OK;
}
+ void addMiddlewareHeaders(ServerContext* context,
Review Comment:
Threading is fine, but calls need to be synchronized (except Read and Write can be done concurrently - no other operations can be called concurrently)
--
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
Re: [PR] GH-39791: [C++][Flight RPC] Flight C++ ServerMiddleware::SendingHeaders call ordering fix [arrow]
Posted by "joellubi (via GitHub)" <gi...@apache.org>.
joellubi commented on PR #40071:
URL: https://github.com/apache/arrow/pull/40071#issuecomment-1971267987
I've been working on the Go implementation of sessions (#40284) and also ran into this issue in a few ways. My general takeaway is that I think it likely makes sense to send metadata which may be dependent on RPC behavior (such as session cookies) in the response trailer rather than the header. In fact, given the [documentation](https://grpc.io/docs/guides/metadata/#headers) on gRPC metadata it's not clear that ignoring changes to headers after RPC handlers have started is in fact broken behavior. There is a separate but related issue, however, which is that not all clients currently check the trailer for metadata. Go and C++ do, but it appears Java only does when an error is caught.
--
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
Re: [PR] GH-39791: [C++][Flight RPC] Flight C++ ServerMiddleware::SendingHeaders call ordering fix [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #40071:
URL: https://github.com/apache/arrow/pull/40071#issuecomment-3547061503
Thank you for your contribution. Unfortunately, this pull request has been marked as stale because it has had no activity in the past 365 days. Please remove the stale label or comment below, or this PR will be closed in 14 days. Feel free to re-open this if it has been closed in error. If you do not have repository permissions to reopen the PR, please tag a maintainer.
--
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
Re: [PR] GH-39791: [C++][Flight RPC] Flight C++ ServerMiddleware::SendingHeaders call ordering fix [arrow]
Posted by "indigophox (via GitHub)" <gi...@apache.org>.
indigophox commented on code in PR #40071:
URL: https://github.com/apache/arrow/pull/40071#discussion_r1491914342
##########
cpp/src/arrow/flight/transport/grpc/grpc_server.cc:
##########
@@ -465,62 +573,84 @@ class GrpcServiceHandler final : public FlightService::Service {
::grpc::Status DoGet(ServerContext* context, const pb::Ticket* request,
ServerWriter<pb::FlightData>* writer) {
GrpcServerCallContext flight_context(context);
- GRPC_RETURN_NOT_GRPC_OK(CheckAuth(FlightMethod::DoGet, context, flight_context));
+ GRPC_CALL_THEN_RETURN_NOT_GRPC_OK(
+ addMiddlewareHeaders(context, flight_context),
+ CheckAuth(FlightMethod::DoGet, context, flight_context, true));
- CHECK_ARG_NOT_NULL(flight_context, request, "ticket cannot be null");
+ CHECK_ARG_NOT_NULL_WITH_CALL(flight_context, request, "ticket cannot be null",
+ addMiddlewareHeaders(context, flight_context));
Ticket ticket;
- SERVICE_RETURN_NOT_OK(flight_context, internal::FromProto(*request, &ticket));
+ SERVICE_CALL_THEN_RETURN_NOT_OK(flight_context,
+ addMiddlewareHeaders(context, flight_context),
+ internal::FromProto(*request, &ticket));
- GetDataStream stream(writer);
- RETURN_WITH_MIDDLEWARE(flight_context,
- impl_->DoGet(flight_context, std::move(ticket), &stream));
+ GetDataStream stream(writer,
+ [&]() { addMiddlewareHeaders(context, flight_context); });
+ RETURN_WITH_CALL_AND_MIDDLEWARE(
+ flight_context, stream.TryCallOnceBeforeWrite(),
+ impl_->DoGet(flight_context, std::move(ticket), &stream));
}
::grpc::Status DoPut(
ServerContext* context,
::grpc::ServerReaderWriter<pb::PutResult, pb::FlightData>* reader) {
GrpcServerCallContext flight_context(context);
- GRPC_RETURN_NOT_GRPC_OK(CheckAuth(FlightMethod::DoPut, context, flight_context));
+ GRPC_CALL_THEN_RETURN_NOT_GRPC_OK(
+ addMiddlewareHeaders(context, flight_context),
+ CheckAuth(FlightMethod::DoPut, context, flight_context, true));
- PutDataStream stream(reader);
- RETURN_WITH_MIDDLEWARE(flight_context, impl_->DoPut(flight_context, &stream));
+ PutDataStream stream(reader,
+ [&]() { addMiddlewareHeaders(context, flight_context); });
+ RETURN_WITH_CALL_AND_MIDDLEWARE(flight_context, stream.TryCallOnceBeforeWrite(),
+ impl_->DoPut(flight_context, &stream));
}
::grpc::Status DoExchange(
ServerContext* context,
::grpc::ServerReaderWriter<pb::FlightData, pb::FlightData>* stream) {
GrpcServerCallContext flight_context(context);
- GRPC_RETURN_NOT_GRPC_OK(CheckAuth(FlightMethod::DoExchange, context, flight_context));
+ GRPC_CALL_THEN_RETURN_NOT_GRPC_OK(
+ addMiddlewareHeaders(context, flight_context),
+ CheckAuth(FlightMethod::DoExchange, context, flight_context, true));
- ExchangeDataStream data_stream(stream);
- RETURN_WITH_MIDDLEWARE(flight_context,
- impl_->DoExchange(flight_context, &data_stream));
+ ExchangeDataStream data_stream(
+ stream, [&]() { addMiddlewareHeaders(context, flight_context); });
+ RETURN_WITH_CALL_AND_MIDDLEWARE(flight_context, data_stream.TryCallOnceBeforeWrite(),
+ impl_->DoExchange(flight_context, &data_stream));
}
::grpc::Status ListActions(ServerContext* context, const pb::Empty* request,
ServerWriter<pb::ActionType>* writer) {
GrpcServerCallContext flight_context(context);
- GRPC_RETURN_NOT_GRPC_OK(
- CheckAuth(FlightMethod::ListActions, context, flight_context));
+ GRPC_CALL_THEN_RETURN_NOT_GRPC_OK(
+ addMiddlewareHeaders(context, flight_context),
+ CheckAuth(FlightMethod::ListActions, context, flight_context, true));
// Retrieve the listing from the implementation
std::vector<ActionType> types;
- SERVICE_RETURN_NOT_OK(flight_context,
- impl_->base()->ListActions(flight_context, &types));
+ auto res = impl_->base()->ListActions(flight_context, &types);
+ addMiddlewareHeaders(context, flight_context);
+ SERVICE_RETURN_NOT_OK(flight_context, res);
RETURN_WITH_MIDDLEWARE(flight_context, WriteStream<ActionType>(types, writer));
}
::grpc::Status DoAction(ServerContext* context, const pb::Action* request,
ServerWriter<pb::Result>* writer) {
GrpcServerCallContext flight_context(context);
- GRPC_RETURN_NOT_GRPC_OK(CheckAuth(FlightMethod::DoAction, context, flight_context));
- CHECK_ARG_NOT_NULL(flight_context, request, "Action cannot be null");
+ GRPC_CALL_THEN_RETURN_NOT_GRPC_OK(
+ addMiddlewareHeaders(context, flight_context),
Review Comment:
Ack.
Related, what do you think about a change of the macro naming to something more like `GRPC_ON_GRPC_NOT_OK_CALL_THEN_RETURN` as this is getting kind of backwards and impossible to read, and especially starting with `SERVICE_CALL_[...]` just parses as if "service call" is what's being talked about, vs `SERVICE_ON_NOT_OK_CALL_THEN_RETURN` i.e. semantically <TYPE>_ON_<CONDITION>_<DO_WHAT>. Not that I'm sure I want to chainsaw the macros any more than I have :)
--
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
Re: [PR] GH-39791: [C++][Flight RPC] Flight C++ ServerMiddleware::SendingHeaders call ordering fix [arrow]
Posted by "indigophox (via GitHub)" <gi...@apache.org>.
indigophox commented on PR #40071:
URL: https://github.com/apache/arrow/pull/40071#issuecomment-1971971522
@lidavidm as you're more familiar with that area do you have any time to peek at the current state of things with gRPC support for proper (not duct-taped on like this) interceptors to handle this as-desired for this issue? If not I can probably find time to dig around, just figured as you're more familiar with the interface it might be a quickie to check if there have been any changes (which I somewhat doubt). TIA
--
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
Re: [PR] GH-39791: [C++][Flight RPC] Flight C++ ServerMiddleware::SendingHeaders call ordering fix [arrow]
Posted by "indigophox (via GitHub)" <gi...@apache.org>.
indigophox commented on PR #40071:
URL: https://github.com/apache/arrow/pull/40071#issuecomment-1971975310
> I've been working on the Go implementation of sessions (#40284) and also ran into this issue in a few ways. My general takeaway is that I think it likely makes sense to send metadata which may be dependent on RPC behavior (such as session cookies) in the response trailer rather than the header. In fact, given the [documentation](https://grpc.io/docs/guides/metadata/#headers) on gRPC metadata it's not clear that ignoring changes to headers after RPC handlers have started is in fact broken behavior. There is a separate but related issue, however, which is that not all clients currently check the trailer for metadata. Go and C++ do, but it appears Java only does when an error is caught.
Unfortunately cookies absolutely have to be set as part of headers not trailers: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Trailer#directives
I'm definitely not saying that this is broken (not-as-documented) behaviour on gRPC's part, but it's inconsistent with Arrow Flight documentation and with other e.g. Java implementations thereof, and also extremely inconvenient (breaking) as the existing behaviour means that RPC handlers—which otherwise have access to the middleware context—can have no influence on Set-Cookie response headers.
--
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
Re: [PR] GH-39791: [C++][Flight RPC] Flight C++ ServerMiddleware::SendingHeaders call ordering fix [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #40071: GH-39791: [C++][Flight RPC] Flight C++ ServerMiddleware::SendingHeaders call ordering fix
URL: https://github.com/apache/arrow/pull/40071
--
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
Re: [PR] GH-39791: [C++][Flight RPC] Flight C++ ServerMiddleware::SendingHeaders call ordering fix [arrow]
Posted by "indigophox (via GitHub)" <gi...@apache.org>.
indigophox commented on code in PR #40071:
URL: https://github.com/apache/arrow/pull/40071#discussion_r1491918244
##########
cpp/src/arrow/flight/transport/grpc/grpc_server.cc:
##########
@@ -336,21 +406,35 @@ class GrpcServiceHandler final : public FlightService::Service {
if (instance != nullptr) {
flight_context.middleware_.push_back(instance);
flight_context.middleware_map_.insert({factory.first, instance});
- instance->SendingHeaders(&outgoing_headers);
}
}
+ // TODO factor this out after fixing all streaming and non-streaming handlers
+ if (!skip_headers) {
+ addMiddlewareHeaders(context, flight_context);
+ }
+
return ::grpc::Status::OK;
}
+ void addMiddlewareHeaders(ServerContext* context,
Review Comment:
And all of that said, to truly fix this issue everywhere we do need to inject it as a hook into the stream wrappers so that it can be called between "most of" the Flight server's handler code and when response(s) are written to the stream—so not just everywhere we return to gRPC or anywhere we can call it explicitly.
--
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
Re: [PR] GH-39791: [C++][Flight RPC] Flight C++ ServerMiddleware::SendingHeaders call ordering fix [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #40071:
URL: https://github.com/apache/arrow/pull/40071#issuecomment-1952993930
Either that, or revisit gRPC's middleware and see if the issue has been fixed so that we're not just fighting the framework
--
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
Re: [PR] GH-39791: [C++][Flight RPC] Flight C++ ServerMiddleware::SendingHeaders call ordering fix [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #40071:
URL: https://github.com/apache/arrow/pull/40071#issuecomment-1971991497
You could chime in here for instance: https://github.com/grpc/grpc/issues/32508#issuecomment-1967321290
OpenTelemetry is another case where gRPC interceptors are artificially limited for similar reasons as here: you can't communicate a value between the interceptor and the RPC handler, so we have to resort to hacks like the one here. Now that we're taking the hack much further, I'm less willing to accommodate gRPC and would rather they fix interceptors to actually be useful.
--
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
Re: [PR] GH-39791: [C++][Flight RPC] Flight C++ ServerMiddleware::SendingHeaders call ordering fix [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #40071:
URL: https://github.com/apache/arrow/pull/40071#issuecomment-1994396825
It appears upstream gRPC is considering fixing interceptors. https://github.com/grpc/grpc/issues/32508#issuecomment-1992885391
--
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
Re: [PR] GH-39791: [C++][Flight RPC] Flight C++ ServerMiddleware::SendingHeaders call ordering fix [arrow]
Posted by "indigophox (via GitHub)" <gi...@apache.org>.
indigophox commented on code in PR #40071:
URL: https://github.com/apache/arrow/pull/40071#discussion_r1491917140
##########
cpp/src/arrow/flight/transport/grpc/grpc_server.cc:
##########
@@ -336,21 +406,35 @@ class GrpcServiceHandler final : public FlightService::Service {
if (instance != nullptr) {
flight_context.middleware_.push_back(instance);
flight_context.middleware_map_.insert({factory.first, instance});
- instance->SendingHeaders(&outgoing_headers);
}
}
+ // TODO factor this out after fixing all streaming and non-streaming handlers
+ if (!skip_headers) {
+ addMiddlewareHeaders(context, flight_context);
+ }
+
return ::grpc::Status::OK;
}
+ void addMiddlewareHeaders(ServerContext* context,
Review Comment:
Do we explicitly prohibit (I don't think so?!) threaded handler implementations? I wanted to ensure that this is blocking so it works correctly there. I did consider creating the blocking call wrapper object in the handlers instead and passing it to the stream wrapper c'tors, but only once I had poked the ugly hole back into the stream wrappers to handle the "RPC handler didn't actually write to the stream and trigger middleware header ingestion" case i.e. the `stream.TryCallOnceBeforeWrite()`. If you think that one is too offensive I will find the time to relocate where the blocking call wrapper is scoped.
All of that said, I could (as you suggest?) roll the blocking call wrapper into AddMiddlewareHeaders but I wanted to avoid the locking dance for the non-streaming cases (not that it matters incredible much I guess).
--
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
Re: [PR] GH-39791: [C++][Flight RPC] Flight C++ ServerMiddleware::SendingHeaders call ordering fix [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #40071:
URL: https://github.com/apache/arrow/pull/40071#issuecomment-1971293431
gRPC does this too: implementations may send the result code in either the trailer or consolidate it with the headers
--
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
Re: [PR] GH-39791: [C++][Flight RPC] Flight C++ ServerMiddleware::SendingHeaders call ordering fix [arrow]
Posted by "indigophox (via GitHub)" <gi...@apache.org>.
indigophox commented on PR #40071:
URL: https://github.com/apache/arrow/pull/40071#issuecomment-1971968979
Unfortunately cookies absolutely have to be set as part of headers not trailers: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Trailer#directives
--
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
Re: [PR] GH-39791: [C++][Flight RPC] Flight C++ ServerMiddleware::SendingHeaders call ordering fix [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #40071:
URL: https://github.com/apache/arrow/pull/40071#issuecomment-1952991899
I'm still not certain why we can't implement sessions as an in-RPC helper (that uses ServerCallContext::AddHeader) instead of trying to do more workarounds for what is ultimately gRPC's broken middleware implementation (since the original implementation here was trying to work around the fact that gRPC middleware aren't allowed to communicate with the RPC handler)
--
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
Re: [PR] GH-39791: [C++][Flight RPC] Flight C++ ServerMiddleware::SendingHeaders call ordering fix [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #40071:
URL: https://github.com/apache/arrow/pull/40071#discussion_r1491823670
##########
cpp/src/arrow/flight/transport/grpc/grpc_server.cc:
##########
@@ -465,62 +573,84 @@ class GrpcServiceHandler final : public FlightService::Service {
::grpc::Status DoGet(ServerContext* context, const pb::Ticket* request,
ServerWriter<pb::FlightData>* writer) {
GrpcServerCallContext flight_context(context);
- GRPC_RETURN_NOT_GRPC_OK(CheckAuth(FlightMethod::DoGet, context, flight_context));
+ GRPC_CALL_THEN_RETURN_NOT_GRPC_OK(
+ addMiddlewareHeaders(context, flight_context),
+ CheckAuth(FlightMethod::DoGet, context, flight_context, true));
- CHECK_ARG_NOT_NULL(flight_context, request, "ticket cannot be null");
+ CHECK_ARG_NOT_NULL_WITH_CALL(flight_context, request, "ticket cannot be null",
+ addMiddlewareHeaders(context, flight_context));
Ticket ticket;
- SERVICE_RETURN_NOT_OK(flight_context, internal::FromProto(*request, &ticket));
+ SERVICE_CALL_THEN_RETURN_NOT_OK(flight_context,
+ addMiddlewareHeaders(context, flight_context),
+ internal::FromProto(*request, &ticket));
- GetDataStream stream(writer);
- RETURN_WITH_MIDDLEWARE(flight_context,
- impl_->DoGet(flight_context, std::move(ticket), &stream));
+ GetDataStream stream(writer,
+ [&]() { addMiddlewareHeaders(context, flight_context); });
+ RETURN_WITH_CALL_AND_MIDDLEWARE(
+ flight_context, stream.TryCallOnceBeforeWrite(),
+ impl_->DoGet(flight_context, std::move(ticket), &stream));
}
::grpc::Status DoPut(
ServerContext* context,
::grpc::ServerReaderWriter<pb::PutResult, pb::FlightData>* reader) {
GrpcServerCallContext flight_context(context);
- GRPC_RETURN_NOT_GRPC_OK(CheckAuth(FlightMethod::DoPut, context, flight_context));
+ GRPC_CALL_THEN_RETURN_NOT_GRPC_OK(
+ addMiddlewareHeaders(context, flight_context),
+ CheckAuth(FlightMethod::DoPut, context, flight_context, true));
- PutDataStream stream(reader);
- RETURN_WITH_MIDDLEWARE(flight_context, impl_->DoPut(flight_context, &stream));
+ PutDataStream stream(reader,
+ [&]() { addMiddlewareHeaders(context, flight_context); });
+ RETURN_WITH_CALL_AND_MIDDLEWARE(flight_context, stream.TryCallOnceBeforeWrite(),
+ impl_->DoPut(flight_context, &stream));
}
::grpc::Status DoExchange(
ServerContext* context,
::grpc::ServerReaderWriter<pb::FlightData, pb::FlightData>* stream) {
GrpcServerCallContext flight_context(context);
- GRPC_RETURN_NOT_GRPC_OK(CheckAuth(FlightMethod::DoExchange, context, flight_context));
+ GRPC_CALL_THEN_RETURN_NOT_GRPC_OK(
+ addMiddlewareHeaders(context, flight_context),
+ CheckAuth(FlightMethod::DoExchange, context, flight_context, true));
- ExchangeDataStream data_stream(stream);
- RETURN_WITH_MIDDLEWARE(flight_context,
- impl_->DoExchange(flight_context, &data_stream));
+ ExchangeDataStream data_stream(
+ stream, [&]() { addMiddlewareHeaders(context, flight_context); });
+ RETURN_WITH_CALL_AND_MIDDLEWARE(flight_context, data_stream.TryCallOnceBeforeWrite(),
+ impl_->DoExchange(flight_context, &data_stream));
}
::grpc::Status ListActions(ServerContext* context, const pb::Empty* request,
ServerWriter<pb::ActionType>* writer) {
GrpcServerCallContext flight_context(context);
- GRPC_RETURN_NOT_GRPC_OK(
- CheckAuth(FlightMethod::ListActions, context, flight_context));
+ GRPC_CALL_THEN_RETURN_NOT_GRPC_OK(
+ addMiddlewareHeaders(context, flight_context),
+ CheckAuth(FlightMethod::ListActions, context, flight_context, true));
// Retrieve the listing from the implementation
std::vector<ActionType> types;
- SERVICE_RETURN_NOT_OK(flight_context,
- impl_->base()->ListActions(flight_context, &types));
+ auto res = impl_->base()->ListActions(flight_context, &types);
+ addMiddlewareHeaders(context, flight_context);
+ SERVICE_RETURN_NOT_OK(flight_context, res);
RETURN_WITH_MIDDLEWARE(flight_context, WriteStream<ActionType>(types, writer));
}
::grpc::Status DoAction(ServerContext* context, const pb::Action* request,
ServerWriter<pb::Result>* writer) {
GrpcServerCallContext flight_context(context);
- GRPC_RETURN_NOT_GRPC_OK(CheckAuth(FlightMethod::DoAction, context, flight_context));
- CHECK_ARG_NOT_NULL(flight_context, request, "Action cannot be null");
+ GRPC_CALL_THEN_RETURN_NOT_GRPC_OK(
+ addMiddlewareHeaders(context, flight_context),
Review Comment:
just a nit, please follow the naming conventions
--
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
Re: [PR] GH-39791: [C++][Flight RPC] Flight C++ ServerMiddleware::SendingHeaders call ordering fix [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #40071:
URL: https://github.com/apache/arrow/pull/40071#issuecomment-1971988728
IIRC, it's basically impossible, but you may have better luck getting answers from the gRPC team than I have. (I think the call filter API lets you do this, but because of various hardcoded things that I saw, basically only one call filter can be registered globally, so that's even more reason that Flight shouldn't touch it.)
Hence, why I've suggested multiple times that for C++, the best way to do this would be to just inline the middleware into the RPC handler, instead of having a formal middleware (then the RPC handler is in full control).
gRPC doesn't necessarily follow HTTP conventions in the first place (example: the HTTP status code is always 200). So that's not necessarily binding. And as mentioned some gRPC implementations already switch between headers/trailers for responses.
--
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
Re: [PR] GH-39791: [C++][Flight RPC] Flight C++ ServerMiddleware::SendingHeaders call ordering fix [arrow]
Posted by "joellubi (via GitHub)" <gi...@apache.org>.
joellubi commented on PR #40071:
URL: https://github.com/apache/arrow/pull/40071#issuecomment-1972073146
> Unfortunately cookies absolutely have to be set as part of headers not trailers: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Trailer#directives
This makes me wonder if cookies are an appropriate mechanism for session passing. How would this generalize to streaming RPC when the session state may not be known until after the headers are set?
--
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