Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Http2Session.streamIdleTimeout should permit being disabled from AbstractHTTP2ServerConnectionFactory #9720

Closed
niloc132 opened this issue May 1, 2023 · 1 comment · Fixed by #10173
Assignees

Comments

@niloc132
Copy link

niloc132 commented May 1, 2023

Jetty version(s)
Jetty 11, including Jetty 11.0.15

Enhancement Description
Timeouts are configurable at several places when starting a Server, but the H2 streamIdleTimeout doesn't support being disabled without also disabling all timeouts on the same connector.

This isn't the typical H2 "keep-alive" timeout, but seems to be something specific to Jetty, allowing a server to prevent long-running streams.

HTTP2Session gets the value for streamIdleTimeout initially from Endpoint.getIdleTimeout(), which in turn gets the value from AbstractConnector.getIdleTimeout() - a general value that is used for effectively all timeouts.

It is also possible to set a more specific stream idle timeout on AbstractHTTP2ServerConnectionFactory's streamIdleTimeout property, but unfortunately any non-positive value will be ignored:
https://github.com/eclipse/jetty.project/blob/d1195b512340e9ab694fe5bb57b5615b437a475a/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/AbstractHTTP2ServerConnectionFactory.java#L268-L274

This makes it tricky to specify that other timeouts are acceptable in their default configuration, but long-running H2 streams to clients might be permitted to have longer delays between messages.

Workaround
As a workaround, it is possible to create a listener on the ServerConnector, and listen for HTTP2ServerConnection instances as they are created, and directly configure the session's timeout value.

        // Override the h2 stream timeout with a specified value
        serverConnector.addEventListener(new Connection.Listener() {
            @Override
            public void onOpened(Connection connection) {
                if (connection instanceof HTTP2ServerConnection) {
                    HTTP2Session session = (HTTP2Session) ((HTTP2Connection) connection).getSession();
                    session.setStreamIdleTimeout(0);// Disable timeout
                }
            }

            @Override
            public void onClosed(Connection connection) {

            }
        });

Related
Note that disabling stream idle timeouts appears to be another workaround for #8405, though that issue is about the client listening to a server stream, and even if the timeout isn't disabled, the stream will not stop after the timeout - instead, the bug will manifest, where onAllDataRead() is called twice, once before the server has finished sending.

Possible fix
It might be possible to simply remove the > 0 check in AbstractHTTP2ServerConnectionFactory.newConnection, either relaxing it to >= 0, or permitting negative values.

@niloc132 niloc132 changed the title Http2Session.streamIdleTimeout should permit being disabled Http2Session.streamIdleTimeout should permit being disabled from AbstractHTTP2ServerConnectionFactory May 1, 2023
sbordet added a commit that referenced this issue Jul 29, 2023
…bled

Now allowing to specify a negative value for AbstractHTTP2ServerConnectionFactory.streamIdleTimeout, while 0 implies to use the default value (from the EndPoint).

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet self-assigned this Jul 29, 2023
@sbordet
Copy link
Contributor

sbordet commented Jul 29, 2023

@niloc132 would you be able to try #10173 and see if it fixes the issue for you?

sbordet added a commit that referenced this issue Jul 31, 2023
…bled

Now allowing to specify a negative value for AbstractHTTP2ServerConnectionFactory.streamIdleTimeout, while 0 implies to use the default value (from the EndPoint).

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants