HTTP/2: add per-stream idle and lifetime timeouts to core multiplexer

[arturobernalg]

This change introduces per-stream idle and lifetime timeouts enforced by the HTTP/2 core multiplexer. Streams that exceed the configured limits are reset with H2StreamTimeoutException, while the underlying HTTP/2 connection remains usable for other streams.

Configuration is exposed via H2Config.setStreamIdleTimeout and setStreamLifetimeTimeout and is disabled by default, so existing users see no behaviour change unless they opt in. H2Stream now tracks creation and last-activity timestamps, and AbstractH2StreamMultiplexer inspects those on I/O events to apply the timeouts without background threads.

[rschmitt]

1. What kinds of timeouts are currently supported with HTTP/2?
2. Is H2Config the correct place to put these options, since they are not conceptually specific to HTTP/2?

[arturobernalg]

1. What kinds of timeouts are currently supported with HTTP/2?

   2. Is H2Config the correct place to put these options, since they are not conceptually specific to HTTP/2?

At the moment we only expose connection-level timeouts (for example via IOReactorConfig socket and session timeouts); there is no existing notion of per-stream timeout in the HTTP/2 code.
These new options are enforced entirely inside the HTTP/2 stream multiplexer, so they are scoped to HTTP/2 stream semantics and therefore live in H2Config.
If we ever want similar per-message timeouts for other protocols, we can add a higher-level abstraction then without changing this API.

[rschmitt]

If we ever want similar per-message timeouts for other protocols, we can add a higher-level abstraction then without changing this API.

What about RequestConfig#setResponseTimeout? The Javadoc seems to indicate that that option is supported everywhere but HTTP/2. Should we start by backfilling multiplexer support for this specific setting?

[arturobernalg]

If we ever want similar per-message timeouts for other protocols, we can add a higher-level abstraction then without changing this API.

What about RequestConfig#setResponseTimeout? The Javadoc seems to indicate that that option is supported everywhere but HTTP/2. Should we start by backfilling multiplexer support for this specific setting?

RequestConfig#setResponseTimeout IMO lives in the client API, while H2Config is a transport-level setting in Core. I would keep this change scoped to Core and then look at wiring responseTimeout onto an appropriate per-stream timeout in the httpclient5 layer as a follow-up

[ok2c]

@arturobernalg @rschmitt This timeout value / these timeout values must be settable / mutable at runtime exactly the same way as Socket read timeout. Just setting an initial value from a config bean is not good enough. At the client level the request execution pipeline could use that API to apply RequestConfig#responseTimeout at the initiation of the message exchange

[arturobernalg]

@arturobernalg @rschmitt This timeout value / these timeout values must be settable / mutable at runtime exactly the same way as Socket read timeout. Just setting an initial value from a config bean is not good enough. At the client level the request execution pipeline could use that API to apply RequestConfig#responseTimeout at the initiation of the message exchange

@ok2c Makes sense. I can treat the timeouts in H2Config as defaults and keep the actual per-stream timeout state on H2Stream.
I will expose a small control API on H2StreamChannel so the client execution pipeline can set / mutate idle / lifetime timeouts at the start of an exchange and map RequestConfig#responseTimeout to the stream lifetime there.

[ok2c]

I can treat the timeouts in H2Config as defaults

@arturobernalg I agree with @rschmitt that is the wrong place for timeout settings. Those are meant to be H2 protocol related configs. Please use connection socket timeout as an initial / default value.

[arturobernalg]

I can treat the timeouts in H2Config as defaults

@arturobernalg I agree with @rschmitt that is the wrong place for timeout settings. Those are meant to be H2 protocol related configs. Please use connection socket timeout as an initial / default value.

@rschmitt @ok2c please another pass

[rschmitt]

I'd like to see HTTP/2 test coverage added to httpclient5-testing/src/test/java/org/apache/hc/client5/testing/async/TestAsyncSocketTimeout.java in the client integration tests. This would provide at least some assurance that we are implementing a consistent contract across the various transports and protocols.

[ok2c]

I'd like to see HTTP/2 test coverage added to httpclient5-testing/src/test/java/org/apache/hc/client5/testing/async/TestAsyncSocketTimeout.java in the client integration tests. This would provide at least some assurance that we are implementing a consistent contract across the various transports and protocols.

@rschmitt That is fair, but the feature would need to have been made available in an alpha release of core or the timeout tests would need to be ported to core.

[arturobernalg]

I'd like to see HTTP/2 test coverage added to httpclient5-testing/src/test/java/org/apache/hc/client5/testing/async/TestAsyncSocketTimeout.java in the client integration tests. This would provide at least some assurance that we are implementing a consistent contract across the various transports and protocols.

@rschmitt I’ve added an async core HTTP/2 socket-timeout test in httpcore5-testing (AsyncH2SocketTimeoutCoreTest) that mirrors the client-side TestAsyncSocketTimeout behaviour. It verifies that a per-request timeout over HTTP/2 results in HttpStreamResetException, so I believe this covers the contract you were asking for – let me know if you had something different in mind.

[ok2c]

@arturobernalg Please rebase this change-set off the latest master and I will review it

[arturobernalg]

@arturobernalg Please rebase this change-set off the latest master and I will review it

@ok2c please take a look

[rschmitt]

@rschmitt I’ve added an async core HTTP/2 socket-timeout test in httpcore5-testing (AsyncH2SocketTimeoutCoreTest) that mirrors the client-side TestAsyncSocketTimeout behaviour. It verifies that a per-request timeout over HTTP/2 results in HttpStreamResetException, so I believe this covers the contract you were asking for – let me know if you had something different in mind.

Ideally I'd like to see contract testing, i.e. the same test cases running for HTTP, HTTPS, h2, and h2c. This ensures that the high-level API behaves consistently across the various implementations; it's also a very effective way of asserting that the various config options are being propagated and applied correctly throughout the client's internals. This is most effectively done through tests in httpclient5 (which has the issue that Oleg pointed out, unless you are backfilling a feature into existing APIs).

[ok2c]

@arturobernalg This looks wrong. The idea is to let the callback from RequestExecutionCommand decide what is to be done upon stream initialization. The StreamControl should just provide a setter.

[ok2c]

@arturobernalg What lifetimeTimeout could be good for? Why do we need it?

[ok2c]

@arturobernalg Please try to minimize changes to H2 protocol code. More functionality can be added incrementally at a later point. Try to keep it small and lean.

[arturobernalg]

@arturobernalg Please try to minimize changes to H2 protocol code. More functionality can be added incrementally at a later point. Try to keep it small and lean.

@ok2c please another pass

[ok2c]

@arturobernalg Better, but please look at all my comments

[arturobernalg]

@o

@arturobernalg Better, but please look at all my comments

@ok2c I’ve updated the patch to reflect your feedback. please let me know if I missed anything.

[ok2c]

@o

@arturobernalg Better, but please look at all my comments

@ok2c I’ve updated the patch to reflect your feedback. please let me know if I missed anything.

@arturobernalg I seem unable to get the message across or articulate it correctly. The patch still does too much and in the wrong places. I am not in favor of applying the changes in their present state. I suggest we drop this pull request and revisit the issue at a later point.