From f1613074bae493a9f87818cb8408d6b679455fec Mon Sep 17 00:00:00 2001 From: Kannan J Date: Thu, 12 Nov 2026 07:55:33 +0000 Subject: [PATCH] servlet: Avoid sending Trailers-Only responses Tomcat does not set END_STREAM on a Trailers-Only HEADERS frame; it terminates the stream with an empty DATA frame instead, which the client rejects as "Received unexpected EOS on empty DATA frame from server", discarding the real status. Always send headers followed by trailers. This fixes flaky TomcatInteropTest failures and re-enables the Tomcat interop and transport tests that were ignored for lack of trailers-only support. Since servlet servers now deliver their own status when the server-side deadline fires first (rather than resetting the stream), AbstractInteropTest.deadlineExceeded also accepts the server's "context timed out" description. AbstractTransportTest's earlyServerClose_noServerHeaders now accepts comma-joined duplicate ASCII trailer values, as basicStream already does, since servlet trailers are set via a Map. TAG=agy CONV=367f1e32-a85a-4e2f-99f3-3ac0e3d14e20 --- .../grpc/internal/AbstractTransportTest.java | 5 ++-- .../integration/AbstractInteropTest.java | 5 +++- .../io/grpc/servlet/ServletServerStream.java | 12 ++++++--- .../io/grpc/servlet/TomcatInteropTest.java | 25 ------------------- .../io/grpc/servlet/TomcatTransportTest.java | 15 ----------- 5 files changed, 14 insertions(+), 48 deletions(-) diff --git a/core/src/testFixtures/java/io/grpc/internal/AbstractTransportTest.java b/core/src/testFixtures/java/io/grpc/internal/AbstractTransportTest.java index ec4e71e07f7..bb9ae53fbc6 100644 --- a/core/src/testFixtures/java/io/grpc/internal/AbstractTransportTest.java +++ b/core/src/testFixtures/java/io/grpc/internal/AbstractTransportTest.java @@ -1103,9 +1103,8 @@ public void earlyServerClose_noServerHeaders() throws Exception { Metadata clientStreamTrailers = clientStreamListener.awaitTrailers(TIMEOUT_MS, TimeUnit.MILLISECONDS); checkClientStatus(status, clientStreamStatus); - assertEquals( - Lists.newArrayList(trailers.getAll(asciiKey)), - Lists.newArrayList(clientStreamTrailers.getAll(asciiKey))); + assertAsciiMetadataValuesEqual( + trailers.getAll(asciiKey), clientStreamTrailers.getAll(asciiKey)); assertEquals( Lists.newArrayList(trailers.getAll(binaryKey)), Lists.newArrayList(clientStreamTrailers.getAll(binaryKey))); diff --git a/interop-testing/src/main/java/io/grpc/testing/integration/AbstractInteropTest.java b/interop-testing/src/main/java/io/grpc/testing/integration/AbstractInteropTest.java index 51295281a90..710027a5a91 100644 --- a/interop-testing/src/main/java/io/grpc/testing/integration/AbstractInteropTest.java +++ b/interop-testing/src/main/java/io/grpc/testing/integration/AbstractInteropTest.java @@ -1113,7 +1113,10 @@ public void deadlineExceeded() throws Exception { Pattern.matches("CallOptions deadline exceeded after .*s. \\[.*\\]", desc) // If server expires first, it'd reset the stream and client would generate a different // message - || desc.startsWith("ClientCall was cancelled at or after deadline.")); + || desc.startsWith("ClientCall was cancelled at or after deadline.") + // If server expires first and sends the status instead of resetting the stream (e.g., + // servlet based servers), the client sees the server's status + || desc.equals("context timed out")); } assertStatsTrace("grpc.testing.TestService/EmptyCall", Status.Code.OK); diff --git a/servlet/src/main/java/io/grpc/servlet/ServletServerStream.java b/servlet/src/main/java/io/grpc/servlet/ServletServerStream.java index 0182f302698..ee0c2701602 100644 --- a/servlet/src/main/java/io/grpc/servlet/ServletServerStream.java +++ b/servlet/src/main/java/io/grpc/servlet/ServletServerStream.java @@ -280,11 +280,15 @@ public void writeTrailers(Metadata trailers, boolean headersSent, Status status) new Object[] {logId, trailers, headersSent, status}); } if (!headersSent) { - writeHeadersToServletResponse(trailers); - } else { - serializeHeaders(trailers, - (k, v) -> trailerSupplier.get().merge(k, v, (oldV, newV) -> oldV + "," + newV)); + // Don't send a Trailers-Only response (status in the response headers). Some containers, + // like Tomcat, don't set END_STREAM on that HEADERS frame and instead terminate the stream + // with an empty DATA frame, which the client treats as a protocol error, discarding the + // status. Send (empty) headers followed by real trailers instead. + writeHeadersToServletResponse(new Metadata()); + resp.setTrailerFields(trailerSupplier); } + serializeHeaders(trailers, + (k, v) -> trailerSupplier.get().merge(k, v, (oldV, newV) -> oldV + "," + newV)); writer.complete(); } diff --git a/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatInteropTest.java b/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatInteropTest.java index d072fea93a1..aabb27583b4 100644 --- a/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatInteropTest.java +++ b/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatInteropTest.java @@ -112,29 +112,4 @@ protected boolean metricsExpected() { @Ignore("Tomcat is broken on client GOAWAY") @Test public void gracefulShutdown() {} - - @Override - @Ignore("Tomcat is not able to send trailer only") - @Test - public void specialStatusMessage() {} - - @Override - @Ignore("Tomcat is not able to send trailer only") - @Test - public void unimplementedMethod() {} - - @Override - @Ignore("Tomcat is not able to send trailer only") - @Test - public void statusCodeAndMessage() {} - - @Override - @Ignore("Tomcat is not able to send trailer only") - @Test - public void emptyStream() {} - - @Override - @Ignore("Tomcat is not able to send trailer only") - @Test - public void timeoutOnSleepingServer() {} } diff --git a/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatTransportTest.java b/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatTransportTest.java index cd73b096ccb..c4805ebccfd 100644 --- a/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatTransportTest.java +++ b/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatTransportTest.java @@ -261,21 +261,6 @@ public void interactionsAfterClientStreamCancelAreNoops() {} @Test public void clientCancel() {} - @Override - @Ignore("Tomcat does not support trailers only") - @Test - public void earlyServerClose_noServerHeaders() {} - - @Override - @Ignore("Tomcat does not support trailers only") - @Test - public void earlyServerClose_serverFailure() {} - - @Override - @Ignore("Tomcat does not support trailers only") - @Test - public void earlyServerClose_serverFailure_withClientCancelOnListenerClosed() {} - @Override @Ignore("regression since bumping grpc v1.46 to v1.53") @Test