From 4945b87d26e2b79d9f5d8c98eccf7c02fbcb2490 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Fri, 22 Nov 2024 16:37:24 -0600 Subject: [PATCH 1/3] Revert "fix: Close Jetty h2 streams with RST_STREAM and no error code (#6401)" This reverts commit 6ada0cb924963a12d12f7fa07ce9cc277ecf2bd9. --- .../servlet/jakarta/AsyncServletOutputStreamWriter.java | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/grpc-java/grpc-servlet-jakarta/src/main/java/io/grpc/servlet/jakarta/AsyncServletOutputStreamWriter.java b/grpc-java/grpc-servlet-jakarta/src/main/java/io/grpc/servlet/jakarta/AsyncServletOutputStreamWriter.java index 9384be40faf..8b2c1da5412 100644 --- a/grpc-java/grpc-servlet-jakarta/src/main/java/io/grpc/servlet/jakarta/AsyncServletOutputStreamWriter.java +++ b/grpc-java/grpc-servlet-jakarta/src/main/java/io/grpc/servlet/jakarta/AsyncServletOutputStreamWriter.java @@ -121,15 +121,9 @@ public boolean isFinestEnabled() { transportState.runOnTransportThread( () -> { transportState.complete(); - // asyncContext.complete(); + asyncContext.complete(); log.fine("call completed"); }); - // Jetty specific fix: When AsyncContext.complete() is called, Jetty sends a RST_STREAM with - // "cancel" error to the client, while other containers send "no error" in this case. Calling - // close() instead on the output stream still sends the RST_STREAM, but with "no error". Note - // that this does the opposite in at least Tomcat, so we're not going to upstream this change. - // See https://github.com/deephaven/deephaven-core/issues/6400 - outputStream.close(); }; this.isReady = () -> outputStream.isReady(); } From ec78befda84e8cdf5d6adbd55c5a6635b9ebeb24 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Fri, 22 Nov 2024 16:55:36 -0600 Subject: [PATCH 2/3] Reapply "fix: Close Jetty h2 streams with RST_STREAM and no error code (#6401)" This reverts commit 4945b87d26e2b79d9f5d8c98eccf7c02fbcb2490. --- .../servlet/jakarta/AsyncServletOutputStreamWriter.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/grpc-java/grpc-servlet-jakarta/src/main/java/io/grpc/servlet/jakarta/AsyncServletOutputStreamWriter.java b/grpc-java/grpc-servlet-jakarta/src/main/java/io/grpc/servlet/jakarta/AsyncServletOutputStreamWriter.java index 8b2c1da5412..9384be40faf 100644 --- a/grpc-java/grpc-servlet-jakarta/src/main/java/io/grpc/servlet/jakarta/AsyncServletOutputStreamWriter.java +++ b/grpc-java/grpc-servlet-jakarta/src/main/java/io/grpc/servlet/jakarta/AsyncServletOutputStreamWriter.java @@ -121,9 +121,15 @@ public boolean isFinestEnabled() { transportState.runOnTransportThread( () -> { transportState.complete(); - asyncContext.complete(); + // asyncContext.complete(); log.fine("call completed"); }); + // Jetty specific fix: When AsyncContext.complete() is called, Jetty sends a RST_STREAM with + // "cancel" error to the client, while other containers send "no error" in this case. Calling + // close() instead on the output stream still sends the RST_STREAM, but with "no error". Note + // that this does the opposite in at least Tomcat, so we're not going to upstream this change. + // See https://github.com/deephaven/deephaven-core/issues/6400 + outputStream.close(); }; this.isReady = () -> outputStream.isReady(); } From 69c75f4dc79ab3dff75c68e2e212133b04086b55 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Fri, 22 Nov 2024 20:24:10 -0600 Subject: [PATCH 3/3] gRPC-web wrapping must write trailer frame before close --- .../jakarta/web/GrpcWebOutputStream.java | 21 +++++++++++++++++-- .../jakarta/web/GrpcWebServletRequest.java | 2 ++ .../jakarta/web/GrpcWebServletResponse.java | 2 +- 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/grpc-java/grpc-servlet-jakarta/src/main/java/io/grpc/servlet/jakarta/web/GrpcWebOutputStream.java b/grpc-java/grpc-servlet-jakarta/src/main/java/io/grpc/servlet/jakarta/web/GrpcWebOutputStream.java index 8ea70c19022..a2f70b7ac47 100644 --- a/grpc-java/grpc-servlet-jakarta/src/main/java/io/grpc/servlet/jakarta/web/GrpcWebOutputStream.java +++ b/grpc-java/grpc-servlet-jakarta/src/main/java/io/grpc/servlet/jakarta/web/GrpcWebOutputStream.java @@ -17,6 +17,7 @@ import jakarta.servlet.WriteListener; import java.io.IOException; +import java.util.concurrent.atomic.AtomicReference; /** * Wraps the usual ServletOutputStream so as to allow downstream writers to use it according to the servlet spec, but @@ -24,13 +25,15 @@ */ public class GrpcWebOutputStream extends ServletOutputStream implements WriteListener { private final ServletOutputStream wrapped; + private final GrpcWebServletResponse grpcWebServletResponse; // Access to these are guarded by synchronized private Runnable waiting; private WriteListener writeListener; - public GrpcWebOutputStream(ServletOutputStream wrapped) { + public GrpcWebOutputStream(ServletOutputStream wrapped, GrpcWebServletResponse grpcWebServletResponse) { this.wrapped = wrapped; + this.grpcWebServletResponse = grpcWebServletResponse; } @Override @@ -97,7 +100,21 @@ public void flush() throws IOException { @Override public void close() throws IOException { - wrapped.close(); + // Since we're a grpc-web response, we must write trailers on our way out as part of close - but trailers + // for grpc-web are a data frame, not HTTP trailers. Call up to the response to write the trailer frame, + // then close the underlying stream. + AtomicReference exception = new AtomicReference<>(); + grpcWebServletResponse.writeTrailers(() -> { + try { + wrapped.close(); + } catch (IOException e) { + exception.set(e); + } + }); + IOException ex = exception.get(); + if (ex != null) { + throw ex; + } } @Override diff --git a/grpc-java/grpc-servlet-jakarta/src/main/java/io/grpc/servlet/jakarta/web/GrpcWebServletRequest.java b/grpc-java/grpc-servlet-jakarta/src/main/java/io/grpc/servlet/jakarta/web/GrpcWebServletRequest.java index 82dfde664ab..66a25d37a83 100644 --- a/grpc-java/grpc-servlet-jakarta/src/main/java/io/grpc/servlet/jakarta/web/GrpcWebServletRequest.java +++ b/grpc-java/grpc-servlet-jakarta/src/main/java/io/grpc/servlet/jakarta/web/GrpcWebServletRequest.java @@ -58,6 +58,8 @@ public AsyncContext startAsync() throws IllegalStateException { public AsyncContext startAsync(ServletRequest servletRequest, ServletResponse servletResponse) throws IllegalStateException { AsyncContext delegate = super.startAsync(servletRequest, servletResponse); + // Note that this anonymous class has no purpose while our workaround for + // https://github.com/deephaven/deephaven-core/issues/6400 is in place. return new DelegatingAsyncContext(delegate) { private void safelyComplete() { try { diff --git a/grpc-java/grpc-servlet-jakarta/src/main/java/io/grpc/servlet/jakarta/web/GrpcWebServletResponse.java b/grpc-java/grpc-servlet-jakarta/src/main/java/io/grpc/servlet/jakarta/web/GrpcWebServletResponse.java index 2306e3e20f5..68affbeb319 100644 --- a/grpc-java/grpc-servlet-jakarta/src/main/java/io/grpc/servlet/jakarta/web/GrpcWebServletResponse.java +++ b/grpc-java/grpc-servlet-jakarta/src/main/java/io/grpc/servlet/jakarta/web/GrpcWebServletResponse.java @@ -60,7 +60,7 @@ public Supplier> getTrailerFields() { public synchronized GrpcWebOutputStream getOutputStream() throws IOException { if (outputStream == null) { // Provide our own output stream instance, so we can control/monitor the write listener - outputStream = new GrpcWebOutputStream(super.getOutputStream()); + outputStream = new GrpcWebOutputStream(super.getOutputStream(), this); } return outputStream; }