[C++][Flight RPC] ServerMiddleware::SendingHeaders called before RPC handler, not "when sending headers"

[indigophox]

Describe the bug, including details regarding any error messages, version, and platform.

The expected behaviour here is as e.g. the Java implementation does things and the docs imply: SendingHeaders is called when headers are to be sent, i.e. after an RPC handler has had a chance to interact with a call-local server middleware instance.

However, in the C++ implementation this is instead called while the call context is being constructed:

arrow/cpp/src/arrow/flight/transport/grpc/grpc_server.cc

Line 339 in 3fe598a

instance->SendingHeaders(&outgoing_headers);

Ideally this should be called during (voluntarily by the RPC handler code, possibly, but that introduces the difficult case where a handler has not triggered header ingestion from middleware and this must be detected etc.

Blocking handlers are simple enough to rectify this for, but streaming handlers (both e.g. DoAction and to a greater extend things like DoGet/DoExchange) present an issue as the header ingestion cannot be triggered after the handler has run as it must happen before streaming to the client commences. I believe the simplest and cleanest solution here is to introduce a shim around the transport-layer result stream listener that fires a one-time callback to handle the header ingestion then unblocks sync and async calls to the listener's on*() callbacks.

The suggested solution is a slight breaking change, but by-and-large it would only break an implementation that assumed that altering C++ middleware would have no further effect on that call's response headers but did so anyways (and then that started working as it should).

Component(s)

C++, FlightRPC

[indigophox]

This gotcha may or may not be worth mentioning in the C++ API docs in the interim

[indigophox]

@lidavidm I'm assuming not but... can you tell me for sure if the inbound data streams for DoPut/DoExchange have any bearing on when GRPC needs its response headers finalized? Thanks!

[lidavidm]

No, IIRC the only requirement is that you can't send any response messages before you send response headers

[indigophox]

Note to self: I need to document that, while this will mostly resolve things, a bad return Status/Result from a non-streaming RPC handler will still skip middleware response header ingestion (less we special-case the SERVICE_RETURN_NOT_OK macro as well).

[indigophox]

@lidavidm while you were on the right track with suggesting moving this out to the gRPC transport handler shims, on closer inspection it's also going to break the exact thing that MiddlewareScenario tests for (response headers working in case of a failed call) unless I/we monkey with or replace usage of the SERVICE_RETURN_NOT_OK macro at e.g.

arrow/cpp/src/arrow/flight/transport/grpc/grpc_server.cc

Lines 522 to 523 in 990e475

	SERVICE_RETURN_NOT_OK(flight_context,
	impl_->base()->DoAction(flight_context, action, &results));

Preference as to how we proceed here? I'm thinking either duplicate or special-case the macro (not so keen on the latter), or (more likely) handle the non-OK return at exactly the spot above via catch-and-"rethrow" i.e. conditionally (or not) invoke the service response macro around the non-OK Status/Result after handling the headers. Any objections to the latter?

[lidavidm]

I think that's fine but it may help to see it in code since I can't quite unpack what's being said

[indigophox]

I think that's fine but it may help to see it in code since I can't quite unpack what's being said

Like so:

https://github.com/indigophox/arrow/blob/e57d9c1c4da3733c952c8d84959a986a5ab9ba0e/cpp/src/arrow/flight/transport/grpc/grpc_server.cc#L414-L417

Full diff:

https://github.com/indigophox/arrow/compare/990e47561..e57d9c1c4/

Only did it for GetFI so far because that's what the MiddlewareScenario integ test hits (and this passes).

There's a bit more monkey business to come to handle the streaming handlers, but I might defer that to another ticket/PR and just handle the non-streamers for now.

[lidavidm]

that seems fine

[indigophox]

Just realizing that this gets messy insofar as skipping middleware response header injection in (at least in the simpler implementation—this can be worked around with a lot of noisy code) all cases except for those where the Flight handler is actually called. In other words, if we fail on CheckAuth()/MakeCallContext() or any of the subsequent CHECK_ARG_NOT_NULL/etc macros we (by default) don't go get response headers from the middleware instance. Maybe not a concern (I don't like the super-noisy check-return-every-time-manually-sort-of-like-we-do-around-the-Flight-handler-call alternative), but what do you think here?

[lidavidm]

We need to preserve the existing behavior