servlet: force always sending end_stream in trailer frame (Fixes #10124)

[hypnoce]

Add option to force servlet transport to send trailers to avoid empty data frame with end_stream flag from servlet containers implementation (currently Tomcat).
fixes #10124

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: hypnoce / name: François JACQUES (6713b8d)

[ejona86]

Hmm... this is a troublesome issue. We might want to fix support for trailers-only with a zero-length data frame following.

For the short-term: nice job on this PR. I think it is pretty solid. But I'm not sure we want to go this way. I'm a hesitant to have the transport introduce the headers, because that is changing the RPC semantics and interceptors wouldn't react.

A potentially easier workaround in terms of semantics and implementation would be "do this same approach in an interceptor." That way it can be positioned such that other interceptors could add headers if they need to. But it would require applications to use the interceptor.

Thoughts?

[hypnoce]

We might want to fix support for trailers-only with a zero-length data frame following.

True. Now it changes a lot the control flows because we need to wait for the next data frame to ensure to handle headers as trailers if end_stream flag is set and this data frame is empty.

nice job on this PR. I think it is pretty solid.

Thanks a lot :)

I'm a hesitant to have the transport introduce the headers, because that is changing the RPC semantics and interceptors wouldn't react.

From what I understood from the gRPC flow, when writeTrailer is called with headerSent=false, that means that no headers were added by any interceptor. Also, some interceptors might decide to directly close the call without calling call.sendHeader. As far as I know, this PR does not break interceptor semantic. Do you have a use case in mind ?

Now we could add an interceptor at the very beginning of the interceptors chain and implement the sendHeader and close methods of the SimpleForwardingServerCall and force sending 'empty' headers if sendHeader was not called upon closing the call.
I can work on a prototype tomorrow. WDYT ?

[ejona86]

As far as I know, this PR does not break interceptor semantic.

On server-side, everything looks normal. But on client-side headers appear and those were not seen on server-side. So client-side can then get confused (e.g., a key should have been in headers, but wasn't).

Now we could add an interceptor at the very beginning of the interceptors chain and implement the sendHeader and close methods of the SimpleForwardingServerCall and force sending 'empty' headers if sendHeader was not called upon closing the call.

Yes, that is what I was suggesting. I think that should turn out to be really easy.

[hypnoce]

On server-side, everything looks normal. But on client-side headers appear and those were not seen on server-side. So client-side can then get confused (e.g., a key should have been in headers, but wasn't).

True, I get the point. The only weird case I can think of is that the client calls ClientCall.Listener#onHeaders with an empty Metadata (stripped from :status, grpc-message and grpc-status keys). Just for my information, is there a case where a key should have been in headers but are in trailers with the transport-level fix ? I'm turning the problem in my head and I cannot think of any reproducible case.

Yes, that is what I was suggesting. I think that should turn out to be really easy.

@ejona86 I changed the PR to use a server interceptor. Obviously the transport level test do fail again but the interop tests do pass. Let me know what you think. Also, it cannot run before any global server interceptors. One final limitation are runtime exceptions. Handling them seems to be a bit tricky when not implemented at transport level.

[ejona86]

I had thought we'd allow the user to use this themselves. They may need to use it two or more times, between various interceptors.

[ejona86]

To be clear here, I thought we'd make this public (and so it'd also need an @ExperimentalApi annotation).

[ejona86]

To be clear here, I thought we'd make this public (and so it'd also need an @ExperimentalApi annotation).

[larry-safran]

Just need to fix formatting of TomcatInteropTest:

Error: eckstyle] [ERROR] /home/runner/work/grpc-java/grpc-java/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatInteropTest.java:40: Extra separation in import group before 'java.io.File' [CustomImportOrder]

Error: eckstyle] [ERROR] /home/runner/work/grpc-java/grpc-java/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatInteropTest.java:40: Wrong lexicographical order for 'java.io.File' import. Should be before 'org.junit.Test'. [CustomImportOrder]
Error: eckstyle] [ERROR] /home/runner/work/grpc-java/grpc-java/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatInteropTest.java:132: Line is longer than 100 characters (found 153). [LineLength]
Error: eckstyle] [ERROR] /home/runner/work/grpc-java/grpc-java/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatInteropTest.java:133: Line is longer than 100 characters (found 121). [LineLength]
Error: eckstyle] [ERROR] /home/runner/work/grpc-java/grpc-java/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatInteropTest.java:147: 'METHOD_DEF' should be separated from previous statement. [EmptyLineSeparator]
Error: eckstyle] [ERROR] /home/runner/work/grpc-java/grpc-java/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatInteropTest.java:148: Line is longer than 100 characters (found 151). [LineLength]

[hypnoce]

@ejona86 Sorry to ping you again. Where do you stand regarding this issue ?

[hypnoce]

@ejona86 I put back the initial fix here: hypnoce@2eda56a
it seems to cover more use cases than the 'interceptor' one. Let me know. Thanks !

[hypnoce]

@ejona86 Do you still think this PR can be merged ? We are maintaining a gRPC servlet fork on our side and going back to the mainstream gRPC would be a plus. Thanks !

[long76]

up vote fo this PR

[trinque]

Friendly ping for merge. It seems like this is both approved and passing.