From 033c895468436699dc5743fd4f451613b805d53c Mon Sep 17 00:00:00 2001 From: hypnoce Date: Wed, 3 May 2023 07:43:13 +0200 Subject: [PATCH] servlet: force always sending end_stream in trailer frame (Fixes #10124) Signed-off-by: hypnoce --- .../grpc/internal/AbstractTransportTest.java | 8 ++--- .../io/grpc/servlet/JettyTransportTest.java | 2 +- .../java/io/grpc/servlet/ServletAdapter.java | 8 +++-- .../io/grpc/servlet/ServletServerBuilder.java | 17 +++++++++- .../io/grpc/servlet/ServletServerStream.java | 31 +++++++++++++------ .../io/grpc/servlet/TomcatInteropTest.java | 27 ++-------------- .../io/grpc/servlet/TomcatTransportTest.java | 18 ++--------- .../grpc/servlet/UndertowTransportTest.java | 3 +- 8 files changed, 56 insertions(+), 58 deletions(-) diff --git a/core/src/testFixtures/java/io/grpc/internal/AbstractTransportTest.java b/core/src/testFixtures/java/io/grpc/internal/AbstractTransportTest.java index 5d7aeca684b..9bca1e3c889 100644 --- a/core/src/testFixtures/java/io/grpc/internal/AbstractTransportTest.java +++ b/core/src/testFixtures/java/io/grpc/internal/AbstractTransportTest.java @@ -1100,11 +1100,11 @@ public void earlyServerClose_noServerHeaders() throws Exception { clientStreamListener.trailers.get(TIMEOUT_MS, TimeUnit.MILLISECONDS); checkClientStatus(status, clientStreamStatus); assertEquals( - Lists.newArrayList(trailers.getAll(asciiKey)), - Lists.newArrayList(clientStreamTrailers.getAll(asciiKey))); + String.join(",", trailers.getAll(asciiKey)), + String.join(",", clientStreamTrailers.getAll(asciiKey))); assertEquals( - Lists.newArrayList(trailers.getAll(binaryKey)), - Lists.newArrayList(clientStreamTrailers.getAll(binaryKey))); + String.join(",", trailers.getAll(binaryKey)), + String.join(",", clientStreamTrailers.getAll(binaryKey))); assertTrue(clientStreamTracer1.getOutboundHeaders()); assertSame(clientStreamTrailers, clientStreamTracer1.getInboundTrailers()); assertSame(clientStreamStatus, clientStreamTracer1.getStatus()); diff --git a/servlet/src/jettyTest/java/io/grpc/servlet/JettyTransportTest.java b/servlet/src/jettyTest/java/io/grpc/servlet/JettyTransportTest.java index e9cb391ea08..d9133b39de6 100644 --- a/servlet/src/jettyTest/java/io/grpc/servlet/JettyTransportTest.java +++ b/servlet/src/jettyTest/java/io/grpc/servlet/JettyTransportTest.java @@ -69,7 +69,7 @@ public void start(ServerListener listener) throws IOException { listener.transportCreated(new ServletServerBuilder.ServerTransportImpl(scheduler)); ServletAdapter adapter = new ServletAdapter(serverTransportListener, streamTracerFactories, - Integer.MAX_VALUE); + Integer.MAX_VALUE, false); GrpcServlet grpcServlet = new GrpcServlet(adapter); jettyServer = new Server(0); diff --git a/servlet/src/main/java/io/grpc/servlet/ServletAdapter.java b/servlet/src/main/java/io/grpc/servlet/ServletAdapter.java index 5a567916f99..f9c42400097 100644 --- a/servlet/src/main/java/io/grpc/servlet/ServletAdapter.java +++ b/servlet/src/main/java/io/grpc/servlet/ServletAdapter.java @@ -77,15 +77,18 @@ public final class ServletAdapter { private final List streamTracerFactories; private final int maxInboundMessageSize; private final Attributes attributes; + private final boolean forceTrailers; ServletAdapter( ServerTransportListener transportListener, List streamTracerFactories, - int maxInboundMessageSize) { + int maxInboundMessageSize, + boolean forceTrailers) { this.transportListener = transportListener; this.streamTracerFactories = streamTracerFactories; this.maxInboundMessageSize = maxInboundMessageSize; attributes = transportListener.transportReady(Attributes.EMPTY); + this.forceTrailers = forceTrailers; } /** @@ -148,7 +151,8 @@ public void doPost(HttpServletRequest req, HttpServletResponse resp) throws IOEx new InetSocketAddress(req.getLocalAddr(), req.getLocalPort())) .build(), getAuthority(req), - logId); + logId, + forceTrailers); transportListener.streamCreated(stream, method, headers); stream.transportState().runOnTransportThread(stream.transportState()::onStreamAllocated); diff --git a/servlet/src/main/java/io/grpc/servlet/ServletServerBuilder.java b/servlet/src/main/java/io/grpc/servlet/ServletServerBuilder.java index 72c4383d273..8fcb4816773 100644 --- a/servlet/src/main/java/io/grpc/servlet/ServletServerBuilder.java +++ b/servlet/src/main/java/io/grpc/servlet/ServletServerBuilder.java @@ -72,6 +72,7 @@ public final class ServletServerBuilder extends ForwardingServerBuilder v + "," + newValue); - trailerSupplier.get().putIfAbsent(key, newValue); + if (forceTrailers) { + writeHeadersToServletResponse(new Metadata()); + resp.setTrailerFields(trailerSupplier); + serializeTrailers(trailers); + } else { + writeHeadersToServletResponse(trailers); } + } else { + serializeTrailers(trailers); } writer.complete(); } + private void serializeTrailers(Metadata trailers) { + byte[][] serializedHeaders = TransportFrameUtil.toHttp2Headers(trailers); + for (int i = 0; i < serializedHeaders.length; i += 2) { + String key = new String(serializedHeaders[i], StandardCharsets.US_ASCII); + String newValue = new String(serializedHeaders[i + 1], StandardCharsets.US_ASCII); + trailerSupplier.get().computeIfPresent(key, (k, v) -> v + "," + newValue); + trailerSupplier.get().putIfAbsent(key, newValue); + } + } + @Override public void cancel(Status status) { if (resp.isCommitted() && Code.DEADLINE_EXCEEDED == status.getCode()) { diff --git a/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatInteropTest.java b/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatInteropTest.java index 1422b5388fd..a345b18ca75 100644 --- a/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatInteropTest.java +++ b/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatInteropTest.java @@ -60,7 +60,9 @@ public static void cleanUp() throws Exception { @Override protected ServerBuilder getServerBuilder() { - return new ServletServerBuilder().maxInboundMessageSize(AbstractInteropTest.MAX_MESSAGE_SIZE); + return new ServletServerBuilder() + .forceTrailers(true) + .maxInboundMessageSize(AbstractInteropTest.MAX_MESSAGE_SIZE); } @Override @@ -113,27 +115,4 @@ protected boolean metricsExpected() { @Test public void gracefulShutdown() {} - // FIXME - @Override - @Ignore("Tomcat is not able to send trailer only") - @Test - public void specialStatusMessage() {} - - // FIXME - @Override - @Ignore("Tomcat is not able to send trailer only") - @Test - public void unimplementedMethod() {} - - // FIXME - @Override - @Ignore("Tomcat is not able to send trailer only") - @Test - public void statusCodeAndMessage() {} - - // FIXME - @Override - @Ignore("Tomcat is not able to send trailer only") - @Test - public void emptyStream() {} } diff --git a/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatTransportTest.java b/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatTransportTest.java index 262036883a9..158fcc6b6e9 100644 --- a/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatTransportTest.java +++ b/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatTransportTest.java @@ -81,7 +81,8 @@ public void start(ServerListener listener) throws IOException { ServerTransportListener serverTransportListener = listener.transportCreated(new ServerTransportImpl(scheduler)); ServletAdapter adapter = - new ServletAdapter(serverTransportListener, streamTracerFactories, Integer.MAX_VALUE); + new ServletAdapter(serverTransportListener, streamTracerFactories, Integer.MAX_VALUE, + true); GrpcServlet grpcServlet = new GrpcServlet(adapter); tomcatServer = new Tomcat(); @@ -255,21 +256,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 diff --git a/servlet/src/undertowTest/java/io/grpc/servlet/UndertowTransportTest.java b/servlet/src/undertowTest/java/io/grpc/servlet/UndertowTransportTest.java index e14c11985de..0e7bce8a329 100644 --- a/servlet/src/undertowTest/java/io/grpc/servlet/UndertowTransportTest.java +++ b/servlet/src/undertowTest/java/io/grpc/servlet/UndertowTransportTest.java @@ -100,7 +100,8 @@ public void start(ServerListener listener) throws IOException { ServerTransportListener serverTransportListener = listener.transportCreated(new ServerTransportImpl(scheduler)); ServletAdapter adapter = - new ServletAdapter(serverTransportListener, streamTracerFactories, Integer.MAX_VALUE); + new ServletAdapter(serverTransportListener, streamTracerFactories, Integer.MAX_VALUE, + false); GrpcServlet grpcServlet = new GrpcServlet(adapter); InstanceFactory instanceFactory = () -> new ImmediateInstanceHandle<>(grpcServlet);