From 81b5ac086bcf7bb87add23ca676302375c926779 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Fri, 14 Jan 2022 14:11:19 -0600 Subject: [PATCH] gRPC websocket streams should close sanely (#1772) Websocket-based gRPC-web streams were not correctly notifying gRPC internals when the stream was finished, this patch removes a poor attempt at that and correctly ends the stream. This diff also adds java.util.logging support, forwarding it to slf4j, and sending slf4j's log level configuration back into java.util.logging. Finally, this patch simplifies directly running main()s in the server project, so that they share configuration with the actual declared gradle application. Fixes #1745 --- .../web/websocket/WebSocketServerStream.java | 5 ---- .../web/websocket/WebsocketStreamImpl.java | 8 +++---- .../src/main/resources/logback.xml | 1 + .../src/main/resources/logback.xml | 1 + .../src/main/resources/logback.xml | 1 + .../src/main/resources/logback.xml | 1 + qst/graphviz/src/main/resources/logback.xml | 1 + server/build.gradle | 1 + server/jetty/build.gradle | 24 ++++++++++++------- .../src/main/resources/logback-debug.xml | 1 + .../src/main/resources/logback-minimal.xml | 1 + server/jetty/src/main/resources/logback.xml | 1 + server/netty/build.gradle | 24 ++++++++++++------- .../src/main/resources/logback-debug.xml | 1 + .../src/main/resources/logback-minimal.xml | 1 + server/netty/src/main/resources/logback.xml | 1 + .../java/io/deephaven/server/runner/Main.java | 5 ++++ 17 files changed, 51 insertions(+), 27 deletions(-) diff --git a/grpc-java/grpc-servlet-websocket-jakarta/src/main/java/io/grpc/servlet/web/websocket/WebSocketServerStream.java b/grpc-java/grpc-servlet-websocket-jakarta/src/main/java/io/grpc/servlet/web/websocket/WebSocketServerStream.java index b4dfa03ed64..2209c46c42b 100644 --- a/grpc-java/grpc-servlet-websocket-jakarta/src/main/java/io/grpc/servlet/web/websocket/WebSocketServerStream.java +++ b/grpc-java/grpc-servlet-websocket-jakarta/src/main/java/io/grpc/servlet/web/websocket/WebSocketServerStream.java @@ -137,11 +137,6 @@ public void onError(Throwable error) { } } - @OnClose - public void onClose(CloseReason closeReason) { - stream.transportReportStatus(Status.CANCELLED);// remote end hung up - } - private String methodName() { return websocketSession.getRequestURI().getPath().substring(1); } diff --git a/grpc-java/grpc-servlet-websocket-jakarta/src/main/java/io/grpc/servlet/web/websocket/WebsocketStreamImpl.java b/grpc-java/grpc-servlet-websocket-jakarta/src/main/java/io/grpc/servlet/web/websocket/WebsocketStreamImpl.java index 94897b815ea..459dbdb2018 100644 --- a/grpc-java/grpc-servlet-websocket-jakarta/src/main/java/io/grpc/servlet/web/websocket/WebsocketStreamImpl.java +++ b/grpc-java/grpc-servlet-websocket-jakarta/src/main/java/io/grpc/servlet/web/websocket/WebsocketStreamImpl.java @@ -127,10 +127,6 @@ public void transportReportStatus(Status status) { transportState().transportReportStatus(status); } - public void complete() { - transportState().complete(); - } - @Override public TransportState transportState() { return transportState; @@ -205,6 +201,7 @@ public void writeFrame(@Nullable WritableBuffer frame, boolean flush, int numMes ByteBuffer.wrap(((ByteArrayWritableBuffer) frame).bytes, 0, frame.readableBytes()); websocketSession.getBasicRemote().sendBinary(payload); + transportState.runOnTransportThread(() -> transportState.onSentBytes(numBytes)); } } catch (IOException e) { @@ -253,6 +250,9 @@ public void writeTrailers(Metadata trailers, boolean headersSent, Status status) } catch (IOException e) { throw Status.fromThrowable(e).asRuntimeException(); } + transportState().runOnTransportThread(() -> { + transportState().complete(); + }); } @Override diff --git a/java-client/barrage-examples/src/main/resources/logback.xml b/java-client/barrage-examples/src/main/resources/logback.xml index 245bdd103c5..3bf193b5a5a 100644 --- a/java-client/barrage-examples/src/main/resources/logback.xml +++ b/java-client/barrage-examples/src/main/resources/logback.xml @@ -1,4 +1,5 @@ + System.err diff --git a/java-client/flight-examples/src/main/resources/logback.xml b/java-client/flight-examples/src/main/resources/logback.xml index 245bdd103c5..3bf193b5a5a 100644 --- a/java-client/flight-examples/src/main/resources/logback.xml +++ b/java-client/flight-examples/src/main/resources/logback.xml @@ -1,4 +1,5 @@ + System.err diff --git a/java-client/session-examples/src/main/resources/logback.xml b/java-client/session-examples/src/main/resources/logback.xml index 245bdd103c5..3bf193b5a5a 100644 --- a/java-client/session-examples/src/main/resources/logback.xml +++ b/java-client/session-examples/src/main/resources/logback.xml @@ -1,4 +1,5 @@ + System.err diff --git a/log-factory/examples/example-logback/src/main/resources/logback.xml b/log-factory/examples/example-logback/src/main/resources/logback.xml index 3dbff87b25e..d5b2e22388c 100644 --- a/log-factory/examples/example-logback/src/main/resources/logback.xml +++ b/log-factory/examples/example-logback/src/main/resources/logback.xml @@ -1,4 +1,5 @@ + %d{HH:mm:ss.SSS} [%thread] %-5level - %msg%n diff --git a/qst/graphviz/src/main/resources/logback.xml b/qst/graphviz/src/main/resources/logback.xml index c9114a669ba..36db185ad80 100644 --- a/qst/graphviz/src/main/resources/logback.xml +++ b/qst/graphviz/src/main/resources/logback.xml @@ -1,4 +1,5 @@ + System.err diff --git a/server/build.gradle b/server/build.gradle index 6d25f701134..7ff7e267ca1 100644 --- a/server/build.gradle +++ b/server/build.gradle @@ -14,6 +14,7 @@ dependencies { implementation project(':proto:proto-backplane-grpc-flight') implementation project(':open-api-lang-tools') implementation project(':log-factory') + Classpaths.inheritSlf4j(project, 'jul-to-slf4j', 'implementation') implementation project(':application-mode') implementation 'com.github.f4b6a3:uuid-creator:3.6.0' diff --git a/server/jetty/build.gradle b/server/jetty/build.gradle index 00a89364106..37653c73b9a 100644 --- a/server/jetty/build.gradle +++ b/server/jetty/build.gradle @@ -41,14 +41,7 @@ distributions { } } -def extraJvmArgs = [] -if (hasProperty('groovy')) { - extraJvmArgs = ['-Ddeephaven.console.type=groovy'] -} - -applicationName = 'start' -mainClassName = 'io.deephaven.server.jetty.JettyMain' -applicationDefaultJvmArgs = [ +def extraJvmArgs = [ '-server', '-XX:+UseG1GC', '-XX:MaxGCPauseMillis=100', @@ -66,6 +59,19 @@ applicationDefaultJvmArgs = [ '-XX:MinRAMPercentage=70.0', // the percentage of system memory that the JVM will use as maximum '-XX:MaxRAMPercentage=80.0', -] + extraJvmArgs +] +if (hasProperty('groovy')) { + extraJvmArgs += ['-Ddeephaven.console.type=groovy'] +} +tasks.withType(JavaExec) { + // This appends to the existing jvm args, so that java-open-nio still takes effect + jvmArgs extraJvmArgs +} +tasks.withType(CreateStartScripts) { + defaultJvmOpts += extraJvmArgs +} + +applicationName = 'start' +mainClassName = 'io.deephaven.server.jetty.JettyMain' apply plugin: 'io.deephaven.java-open-nio' diff --git a/server/jetty/src/main/resources/logback-debug.xml b/server/jetty/src/main/resources/logback-debug.xml index 661139f9a0a..d9580aae98e 100644 --- a/server/jetty/src/main/resources/logback-debug.xml +++ b/server/jetty/src/main/resources/logback-debug.xml @@ -1,4 +1,5 @@ + diff --git a/server/jetty/src/main/resources/logback.xml b/server/jetty/src/main/resources/logback.xml index 8c8c3c90afd..f69901b2c78 100644 --- a/server/jetty/src/main/resources/logback.xml +++ b/server/jetty/src/main/resources/logback.xml @@ -1,4 +1,5 @@ + diff --git a/server/netty/src/main/resources/logback.xml b/server/netty/src/main/resources/logback.xml index 8c8c3c90afd..f69901b2c78 100644 --- a/server/netty/src/main/resources/logback.xml +++ b/server/netty/src/main/resources/logback.xml @@ -1,4 +1,5 @@ +