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