GH-39791: [C++][Flight RPC] Flight C++ ServerMiddleware::SendingHeaders call ordering fix

[indigophox]

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.

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.

Are these changes tested?

Existing integration tests pass.

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.

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.

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

[github-actions]

⚠️ GitHub issue #39791 has been automatically assigned in GitHub to PR creator.

[lidavidm]

just a nit, please follow the naming conventions

[indigophox]

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 ON_<DO_WHAT>. Not that I'm sure I want to chainsaw the macros any more than I have :)

[lidavidm]

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?

[indigophox]

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).

[indigophox]

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.

[lidavidm]

Threading is fine, but calls need to be synchronized (except Read and Write can be done concurrently - no other operations can be called concurrently)

[lidavidm]

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)

[lidavidm]

Either that, or revisit gRPC's middleware and see if the issue has been fixed so that we're not just fighting the framework

[joellubi]

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 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.

[lidavidm]

gRPC does this too: implementations may send the result code in either the trailer or consolidate it with the headers

[indigophox]

@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

[indigophox]

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 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.

[lidavidm]

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.

[lidavidm]

You could chime in here for instance: grpc/grpc#32508 (comment)

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.

[joellubi]

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?

[lidavidm]

It appears upstream gRPC is considering fixing interceptors. grpc/grpc#32508 (comment)