Fix grpc status headers

[FliegenKLATSCH]

Fixes #2961

Headers are only set, if they are received.
In case the status comes with the first header and is an error response.setComplete() is called to set endStream=true and prevent an empty DATA frame being sent. This would lead to Received unexpected EOS on DATA frame from server errors otherwise on the grpc client side.

[pivotal-cla]

@FliegenKLATSCH Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-cla]

@FliegenKLATSCH Thank you for signing the Contributor License Agreement!

[spencergibb]

Can you add a test? @abelsromero @Albertoimpl can you review

[FliegenKLATSCH]

@spencergibb Any hints on how I could set trailing headers for a test case? Doesn't seem to be easy :/
(The patch is running in our production environment since half a year, without any issue.)

[spencergibb]

I don't. Let's wait for my co-workers tagged above to respond

[Albertoimpl]

Thanks for the ping @spencergibb, and thanks a lot @FliegenKLATSCH for the contribution and for wanting to improve our tests around it.
To add custom headers and trailers we can add to our integration-test application a custom interceptor.

For headers, it should be something like this: https://github.com/grpc/grpc-java/blob/master/examples/src/main/java/io/grpc/examples/header/HeaderServerInterceptor.java

For trailers what you see in the close: https://github.com/grpc/grpc-java/blob/master/interop-testing/src/main/java/io/grpc/testing/integration/TestServiceImpl.java#L587-L590

[FliegenKLATSCH]

Thank you! Ping for review @Albertoimpl @abelsromero

[Albertoimpl]

These assertions should cover the case, thanks @FliegenKLATSCH!

[FliegenKLATSCH]

@spencergibb Any intentions to merge this? Thank you!

[dsyer]

The test case looks useful but I don't see any changes in the implementation. What am I missing? There are some changes in Reactor Netty that will make fixing #3783 easier, so we should take the test from this PR and leave the implementation changes to be replaced by something that actually uses the trailer headers in the client response (see #3855).

[FliegenKLATSCH]

?!
d38431f
!!!

[dsyer]

What's your point @FliegenKLATSCH? Nevermind, I get it:

if (headers.containsKey(GRPC_STATUS_HEADER)) {
		if (!"0".equals(headers.getFirst(GRPC_STATUS_HEADER))) {
			response.setComplete(); // avoid empty DATA frame
		}
	}

This is new. You could have said. It might still be useful along with the test (or it might have been fixed incidentally by the Reactor Netty changes).

[FliegenKLATSCH]

What you pointed out is actually more or less just a "cosmetic" fix. Not sure if there would be any effect, if there is an empty addtional frame or not.
The real fix is in the rest of the commit. We're actually subscribing to the trailing headers from the server now, and forwarding them to the client - instead of always setting/adding the/a status header.

[dsyer]

I added your test to #3855, and it is indeed fixed by the Reactor Netty changes. If you want an author tag, we usually use "Firstname Lastname", so let me know.

[FliegenKLATSCH]

No, I guess I am fine if that works. Just wondering if we're getting all the trailing headers from a stream, if we're just looking once at trailerHeaders() or if there could be some timing issues in case there's lots of data frames or slow network.
Thank you for moving this topic forward.

[dsyer]

I added a streaming service to the integration tests in #3855. Seems to work. Can't say if there would be complications in a slow network.

[violetagg]

No, I guess I am fine if that works. Just wondering if we're getting all the trailing headers from a stream, if we're just looking once at trailerHeaders() or if there could be some timing issues in case there's lots of data frames or slow network. Thank you for moving this topic forward.

What do you mean with lots of data frames or?
A typical HTTP/2 response is (https://datatracker.ietf.org/doc/html/rfc9113#section-8.1):

• headers frame (zero or more continuation frames)
• zero or more data frames
• headers frame with trailer headers (zero or more continuation frames)

Reactor Netty collects the trailer headers and emits them via reactor.netty.http.client.HttpClientResponse#trailerHeaders

The proposed fix forwards the response data then forwards the trailer headers and then completes.

[FliegenKLATSCH]

Exactly. But what happens if there is some delay between the data frames and the trailer headers? Because of packet loss or whatever reason. The code would check, if there are any, Reactor Netty might say no, because it simply did not receive them yet, or not all of them. It might be a theoretical issue, or I might be wrong.. 🤷‍♂️

[violetagg]

Exactly. But what happens if there is some delay between the data frames and the trailer headers? Because of packet loss or whatever reason. The code would check, if there are any, Reactor Netty might say no, because it simply did not receive them yet, or not all of them. It might be a theoretical issue, or I might be wrong.. 🤷‍♂️

No, it is not how it works:
The trailer headers are coming with the very last http content, only then we will emit them i.e. we cannot start emitting trailer headers while there is data and we will emit them only once hence Mono<HttpHeaders>.
Once you subscribe to Mono<HttpHeaders> you either receive them or you receive completion signal.

The proposed change is in #3855 in NettyWriteResponseFilter.java

HttpClientResponse httpClientResponse = exchange.getAttribute(CLIENT_RESPONSE_ATTR);
Mono<Void> write = (isStreamingMediaType(contentType)
		? response.writeAndFlushWith(body.map(Flux::just))
		: response.writeWith(body));
return write.then(TrailerHeadersFilter.filter(getHeadersFilters(), exchange, httpClientResponse)).then();

The code would check, if there are any, Reactor Netty might say no

It works the opposite: the one who is interested subscribes, when Reactor Netty has the trailer headers it will emit them.

[FliegenKLATSCH]

Ah the Mono comes from Netty.. overlooked that, thought we would get just a list there.. Thank you for the clarification!