Disable idle stream timeout, allow it to be configured downstream #3742

niloc132 · 2023-04-26T13:43:46Z

niloc132 · 2023-04-26T13:47:54Z

server/jetty/src/main/java/io/deephaven/server/jetty/JettyConfig.java

+     *
+     */
+    public int http2StreamIdleTimeoutOrDefault() {
+        return http2StreamIdleTimeout().orElse(0);


Need to fix the Javadoc here. The default is defined here, but I intend to also define it in the default .prop file:

Defining it here prevents downstream breakage in applications built on top of DHC, while still leaving a reasonable default, without changing any Java or properties files

Defining it in the .prop file allows it to be passed to a client if so configured (I don't intend to enable that by default at this time), so that in theory if a deployment sets a concrete value here, clients can be informed at what time their streams will end if they've had no traffic.

Ideally we would solve this config situation differently, but I think this is a good compromise?

Does 0 explicitly mean "don't ever idle timeout"?

Any non-positive number is a legal value, and interpreted as "no timeout". Salient bit of Jetty:

long streamIdleTimeout = getStreamIdleTimeout(); if (streamIdleTimeout > 0) session.setStreamIdleTimeout(streamIdleTimeout);

Thanks for clearing up that I was misreading this. I've updated the patch now to directly set our preferred timeout directly on each Http2 connection/session, rather than attempt to do it globally. I'll follow up with a bug report slash feature request to Jetty to see if this could be easier to use?

devinrsmith · 2023-04-26T15:46:50Z

server/jetty/src/main/java/io/deephaven/server/jetty/JettyConfig.java

@@ -128,6 +134,15 @@ public int port() {
    @Nullable
    public abstract Boolean http1();

+    public abstract OptionalInt http2StreamIdleTimeout();


Can we make this an Optional<Duration> instead? (I was going to suggest OptionalLong to match #setStreamIdleTimeout(long), but think Duration is better.)

We can also have a @Check annotation to ensure the value makes sense (ie, I'm assuming negative timeouts should not be allowed.)

Technically negative values are allowed.

Can this be Optional<Duration> instead? (Can still parse as long millis, that's fine.)

I'd still vote no, unless we have a consumer that takes Duration (for example, move to ServerConfig and pass to a matching method there to use it in netty... which doesnt exist, and if it did, it would be long+TimeUnit).

Otherwise we're just building up extra checks to turn a number (as is our convention in every other timeout/duration) into an object, then right back into a number again.

Following jetty (and ServerBuilder), I will change this to a long, and add a Millis suffix to the name.

devinrsmith · 2023-04-26T15:48:24Z

server/jetty/src/main/java/io/deephaven/server/jetty/JettyConfig.java

+     *
+     */
+    public int http2StreamIdleTimeoutOrDefault() {
+        return http2StreamIdleTimeout().orElse(0);


Does 0 explicitly mean "don't ever idle timeout"?

devinrsmith · 2023-04-26T15:50:24Z

server/jetty/src/main/java/io/deephaven/server/jetty/JettyConfig.java

@@ -26,6 +27,7 @@ public abstract class JettyConfig implements ServerConfig {
    public static final int DEFAULT_PLAINTEXT_PORT = 10000;
    public static final String HTTP_WEBSOCKETS = "http.websockets";
    public static final String HTTP_HTTP1 = "http.http1";
+    public static final String HTTP_STREAM_TIMEOUT = "http2.stream.idleTimeout";


I think we should call this http2.stream.idleTimeoutMillis, or leave as-is and expect parsing to be done via java.time.Duration#parse

I think I like Duration strings rather than big ints, but I'm hesitant to change this since we are pretty uniformly millis for other configs at this time.

Should we call this http2.stream.idleTimeoutMillis?

devinrsmith · 2023-04-27T18:37:56Z

Afaict, this PR does not fix the issue for me. Looking at the code, it seems that org.eclipse.jetty.http2.HTTP2Session#setStreamIdleTimeout may essentially be a no-op w/ 0, or supplanted by other means of configuring it. (It's likely that it's an oversight from jetty embedded configurability, or we are trying to configuring jetty incorrectly.)

devinrsmith · 2023-04-28T14:05:52Z

Note: setting org.eclipse.jetty.http2.HTTP2Session#setStreamIdleTimeout to > 0 seems to work, in the sense that it changes the 30s timeout we are seeing; it's just that 0 doesn't take.

devinrsmith · 2023-04-28T20:47:07Z

server/jetty/src/main/java/io/deephaven/server/jetty/JettyBackedGrpcServer.java

+                if (connection instanceof HTTP2ServerConnection) {
+                    HTTP2Session session = (HTTP2Session) ((HTTP2Connection) connection).getSession();
+                    session.setStreamIdleTimeout(config.http2StreamIdleTimeoutOrDefault());
+                }


I think a follow-up ticket to jetty would be good.

devinrsmith · 2023-04-28T20:47:38Z

server/jetty/src/main/java/io/deephaven/server/jetty/JettyConfig.java

@@ -26,6 +27,7 @@ public abstract class JettyConfig implements ServerConfig {
    public static final int DEFAULT_PLAINTEXT_PORT = 10000;
    public static final String HTTP_WEBSOCKETS = "http.websockets";
    public static final String HTTP_HTTP1 = "http.http1";
+    public static final String HTTP_STREAM_TIMEOUT = "http2.stream.idleTimeout";


Should we call this http2.stream.idleTimeoutMillis?

devinrsmith · 2023-04-28T20:48:24Z

server/jetty/src/main/java/io/deephaven/server/jetty/JettyConfig.java

@@ -128,6 +134,15 @@ public int port() {
    @Nullable
    public abstract Boolean http1();

+    public abstract OptionalInt http2StreamIdleTimeout();


Can this be Optional<Duration> instead? (Can still parse as long millis, that's fine.)

devinrsmith · 2023-05-01T16:46:26Z

server/jetty/src/main/java/io/deephaven/server/jetty/JettyConfig.java

@@ -104,6 +106,9 @@ public static Builder buildFromConfig(Configuration config) {
        if (httpHttp1 != null) {
            builder.http1(Boolean.parseBoolean(httpHttp1));
        }
+        if (h2StreamIdleTimeout != null) {
+            builder.http2StreamIdleTimeout(Integer.parseInt(h2StreamIdleTimeout));


Long.parseLong?

nbauernfeind

No additional comments from me.

Disable idle stream timeout, allow it to be configured downstream

6e0df89

Fixes deephaven#3733

niloc132 added grpc jetty labels Apr 26, 2023

niloc132 added this to the Apr 2023 milestone Apr 26, 2023

niloc132 requested review from nbauernfeind and devinrsmith April 26, 2023 13:43

niloc132 added configuration NoDocumentationNeeded ReleaseNotesNeeded Release notes are needed labels Apr 26, 2023

niloc132 commented Apr 26, 2023

View reviewed changes

devinrsmith reviewed Apr 26, 2023

View reviewed changes

devinrsmith added the release blocker A bug/behavior that puts is below the "good enough" threshold to release. label Apr 27, 2023

devinrsmith assigned niloc132 Apr 27, 2023

niloc132 added 2 commits April 28, 2023 12:33

Apply our stream timeout per http2 connection

a1497f8

Added a comment on an old workaround

e42260f

devinrsmith reviewed Apr 28, 2023

View reviewed changes

Javadoc, long instead of int, prop name conventions match for units

3dfb448

niloc132 requested a review from devinrsmith May 1, 2023 16:16

devinrsmith requested changes May 1, 2023

View reviewed changes

nbauernfeind reviewed May 1, 2023

View reviewed changes

Replace remaining int with long

53e73ff

devinrsmith approved these changes May 1, 2023

View reviewed changes

niloc132 merged commit 8ec7ac9 into deephaven:main May 2, 2023

github-actions bot locked and limited conversation to collaborators May 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable idle stream timeout, allow it to be configured downstream #3742

Disable idle stream timeout, allow it to be configured downstream #3742

niloc132 commented Apr 26, 2023

niloc132 Apr 26, 2023

devinrsmith Apr 26, 2023

niloc132 Apr 27, 2023 •

edited

Loading

niloc132 Apr 28, 2023

devinrsmith Apr 28, 2023

devinrsmith Apr 26, 2023

devinrsmith Apr 26, 2023

niloc132 Apr 27, 2023

devinrsmith Apr 28, 2023

niloc132 May 1, 2023

devinrsmith Apr 26, 2023

devinrsmith Apr 26, 2023

niloc132 Apr 27, 2023

devinrsmith Apr 28, 2023

devinrsmith commented Apr 27, 2023

devinrsmith commented Apr 28, 2023

devinrsmith Apr 28, 2023

devinrsmith Apr 28, 2023

devinrsmith Apr 28, 2023

devinrsmith May 1, 2023

nbauernfeind left a comment

Disable idle stream timeout, allow it to be configured downstream #3742

Disable idle stream timeout, allow it to be configured downstream #3742

Conversation

niloc132 commented Apr 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niloc132 Apr 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinrsmith commented Apr 27, 2023

devinrsmith commented Apr 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbauernfeind left a comment

Choose a reason for hiding this comment

niloc132 Apr 27, 2023 •

edited

Loading