pjfanning commented on PR #1320: URL: https://github.com/apache/pekko-http/pull/1320#issuecomment-5887471887
Thanks @Rayan-and-beyond for picking this up — the change is nicely contained, and it's great to see docs and tests included. A few further concerns beyond the jitter one above: **1. The setting shouldn't go through `internalSettings`** `persistent-connection-max-age` is a public config key, but it's carried as `Some(Http2PersistentConnectionSettings(...))` in `Http2ClientSettings.internalSettings`, which is intended as a placeholder for internal things like custom strategies. Consequences: - Anyone calling `withInternalSettings(...)` with some other internal setting silently loses the max age (the `collect` in `PersistentConnection.apply` then falls back to `Duration.Zero`). - There's no programmatic API (`persistentConnectionMaxAge` / `withPersistentConnectionMaxAge`) and no javadsl counterpart, unlike every other key in that config block. - `internalSettings` changes from `None` to always `Some(...)` by default. Could this be a proper field on `Http2ClientSettings` (scaladsl + javadsl), with a MiMa excludes file? `maxHeaderListSize` in 1.4.1 (`http-core/src/main/mima-filters/1.4.x.backwards.excludes/http2-max-header-list-size.excludes`) and the server-side #1316 are good precedents. **2. A buffered request can be dropped if upstream completes while retiring** During retirement the replacement `InHandler` keeps one pushed request in the `requestIn` slot and ignores `onUpstreamFinish`. If the user pushes one more request and then completes the request source, once the old connection drains `onDisconnected` sees `isClosed(requestIn)` and calls `completeStage()`. The inlet reports closed while the un-grabbed element is still in the slot, so that request never gets dispatched or answered. I think `onDisconnected` needs to check `isAvailable(requestIn)` before completing, and reconnect to send it. A test for "request pushed after the age timer fires, then upstream completes" would cover this. **3. Retirement is break-before-make, which should at least be documented** Once retirement starts, new requests are backpressured until all in-flight streams finish or `completion-timeout` (default 3s) forcibly closes the old connection, and then a new connection (TCP + TLS + H2 handshake) is established. So: - every retirement adds the drain time plus the connection setup time to new requests' latency; - long-lived streams (gRPC server-streaming, SSE, large downloads) are cut off after `completion-timeout`. The docs say `completion-timeout` "bounds how long an in-flight request may delay retirement", but don't make it clear that those requests are terminated. Make-before-break (open the new connection, move dispatch to it, let the old one drain) would avoid both issues. If that's out of scope for this PR, please spell out the trade-off in `reference.conf` and `http2.md`. **4. Smaller points** - `maxConnectionAgeCallback: Option[() => Unit]` at stage level is only a way to reach `Connected.retire()` from `onTimer`. Tracking the current `Connected` state directly would be simpler. - The changes to `Unconnected.onPull` and the `onDisconnected` reconnect branch (connect when `requestIn` already holds an element) also affect the normal failure/reconnect path, not just retirement. It would be worth mentioning that in the PR description, and possibly adding a targeted test. - The new test relies on tight timings (300ms age, `expectNoRequest(500.millis)` / `expectNoRequest(100.millis)`), which may be flaky on slow CI. There's also no test for retiring an idle connection (no in-flight requests), then reconnecting lazily on the next request. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
