Skip to content

Commit

Permalink
gRPC websocket streams should close sanely (#1772)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
niloc132 authored Jan 14, 2022
1 parent ba6f16a commit 81b5ac0
Show file tree
Hide file tree
Showing 17 changed files with 51 additions and 27 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,10 +127,6 @@ public void transportReportStatus(Status status) {
transportState().transportReportStatus(status);
}

public void complete() {
transportState().complete();
}

@Override
public TransportState transportState() {
return transportState;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<configuration debug="false">
<contextListener class="ch.qos.logback.classic.jul.LevelChangePropagator"/>
<appender name="STDERR" class="ch.qos.logback.core.ConsoleAppender">
<target>System.err</target>
<encoder>
Expand Down
1 change: 1 addition & 0 deletions java-client/flight-examples/src/main/resources/logback.xml
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<configuration debug="false">
<contextListener class="ch.qos.logback.classic.jul.LevelChangePropagator"/>
<appender name="STDERR" class="ch.qos.logback.core.ConsoleAppender">
<target>System.err</target>
<encoder>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<configuration debug="false">
<contextListener class="ch.qos.logback.classic.jul.LevelChangePropagator"/>
<appender name="STDERR" class="ch.qos.logback.core.ConsoleAppender">
<target>System.err</target>
<encoder>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<configuration debug="true">
<contextListener class="ch.qos.logback.classic.jul.LevelChangePropagator"/>
<appender name="STDOUT" class="ch.qos.logback.core.ConsoleAppender">
<encoder>
<pattern>%d{HH:mm:ss.SSS} [%thread] %-5level - %msg%n</pattern>
Expand Down
1 change: 1 addition & 0 deletions qst/graphviz/src/main/resources/logback.xml
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<configuration debug="false">
<contextListener class="ch.qos.logback.classic.jul.LevelChangePropagator"/>
<appender name="STDERR" class="ch.qos.logback.core.ConsoleAppender">
<target>System.err</target>
<encoder>
Expand Down
1 change: 1 addition & 0 deletions server/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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'

Expand Down
24 changes: 15 additions & 9 deletions server/jetty/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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'
1 change: 1 addition & 0 deletions server/jetty/src/main/resources/logback-debug.xml
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<configuration debug="true">
<contextListener class="ch.qos.logback.classic.jul.LevelChangePropagator"/>

<!-- System.out / System.err may be redirected (and sent to LogBuffer).
By referencing PrintStreamGlobalsConsole, we can be sure to avoid that potential redirection
Expand Down
1 change: 1 addition & 0 deletions server/jetty/src/main/resources/logback-minimal.xml
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<configuration debug="false">
<contextListener class="ch.qos.logback.classic.jul.LevelChangePropagator"/>

<!-- LogBuffer is a ring-buffer of recent log messages. It allows clients (such as the web console)
to subscribe and display information. -->
Expand Down
1 change: 1 addition & 0 deletions server/jetty/src/main/resources/logback.xml
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<configuration debug="false">
<contextListener class="ch.qos.logback.classic.jul.LevelChangePropagator"/>

<!-- System.out / System.err may be redirected (and captured by LogBuffer).
By referencing PrintStreamGlobalsConsole, we can be sure that we avoid that redirection so
Expand Down
24 changes: 15 additions & 9 deletions server/netty/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,7 @@ distributions {
}
}

def extraJvmArgs = []
if (hasProperty('groovy')) {
extraJvmArgs = ['-Ddeephaven.console.type=groovy']
}

applicationName = 'start'
mainClassName = 'io.deephaven.server.netty.NettyMain'
applicationDefaultJvmArgs = [
def extraJvmArgs = [
'-server',
'-XX:+UseG1GC',
'-XX:MaxGCPauseMillis=100',
Expand All @@ -58,7 +51,20 @@ 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.netty.NettyMain'

artifacts {
applicationDist project.tasks.findByName('distTar')
Expand Down
1 change: 1 addition & 0 deletions server/netty/src/main/resources/logback-debug.xml
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<configuration debug="true">
<contextListener class="ch.qos.logback.classic.jul.LevelChangePropagator"/>

<!-- System.out / System.err may be redirected (and sent to LogBuffer).
By referencing PrintStreamGlobalsConsole, we can be sure to avoid that potential redirection
Expand Down
1 change: 1 addition & 0 deletions server/netty/src/main/resources/logback-minimal.xml
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<configuration debug="false">
<contextListener class="ch.qos.logback.classic.jul.LevelChangePropagator"/>

<!-- LogBuffer is a ring-buffer of recent log messages. It allows clients (such as the web console)
to subscribe and display information. -->
Expand Down
1 change: 1 addition & 0 deletions server/netty/src/main/resources/logback.xml
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<configuration debug="false">
<contextListener class="ch.qos.logback.classic.jul.LevelChangePropagator"/>

<!-- System.out / System.err may be redirected (and captured by LogBuffer).
By referencing PrintStreamGlobalsConsole, we can be sure that we avoid that redirection so
Expand Down
5 changes: 5 additions & 0 deletions server/src/main/java/io/deephaven/server/runner/Main.java
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import io.deephaven.io.logger.Logger;
import io.deephaven.util.process.ProcessEnvironment;
import org.jetbrains.annotations.NotNull;
import org.slf4j.bridge.SLF4JBridgeHandler;

import java.io.FileReader;
import java.io.IOException;
Expand Down Expand Up @@ -62,6 +63,10 @@ public static Configuration init(String[] args, Class<?> mainClass) throws IOExc

final Configuration config = Configuration.getInstance();

// After logging and config are working, redirect any future JUL logging to SLF4J
SLF4JBridgeHandler.removeHandlersForRootLogger();
SLF4JBridgeHandler.install();

// Push our log to ProcessEnvironment, so that any parts of the system relying on ProcessEnvironment
// instead of LoggerFactory can get the correct logger.
final ProcessEnvironment processEnvironment =
Expand Down

0 comments on commit 81b5ac0

Please sign in to comment.