pjfanning commented on PR #1316: URL: https://github.com/apache/pekko-http/pull/1316#issuecomment-5819868670
Thanks for this, the feature is useful and reusing the existing graceful-termination path is the right approach. The jitter handling and the corrected termination debug log look good. **Scope:** we'd like to treat this as a 2.0.0-only change for now. It is a new feature (new public settings and methods on `Http2ServerSettings`, new config keys, new connection-lifecycle behaviour) rather than a fix, so it doesn't fit a 1.4.x patch release. The max-header-list-size backport (#1247) was an exception because it addressed unbounded memory use from never-finished CONTINUATION sequences (RFC 9113 ยง10.5). Please close #1317. On your `@since` question: the tag should name the first release that carries the API, so `@since 2.0.0` and the `2.0.x.backwards.excludes` filter file are correct here as written. ### Issues to address **1. The drain after max-connection-age is unbounded (`Http2Demux.scala`, `MaxConnectionAge` timer)** Before this PR, every call to `triggerTermination` passed a finite deadline (its only caller was `terminate(deadline: FiniteDuration)`), so a draining connection always had a `CompletionTimeout` that force-closes it. `triggerTermination(Duration.Inf)` introduces the first drain with no deadline: a long-running stream (e.g. gRPC server streaming) keeps the connection open indefinitely, which also defeats the rebalancing this feature is for. Please add a `max-connection-age-grace` setting with a **finite** default (grpc-java has the equivalent `maxConnectionAgeGrace`), pass it from the `MaxConnectionAge` timer, and keep `triggerTermination(deadline: FiniteDuration)` unchanged. The widened `Duration` signature and the `deadline match` then go away. **2. A later `ServerBinding.terminate(hardDeadline)` is ignored on a connection that is already draining** `triggerTermination` is guarded by `if (!terminating)`. Once the max-connection-age drain has started, a binding-level `terminate(hardDeadline)` (e.g. from CoordinatedShutdown) is a no-op for that connection. For HTTP/2 the demux logic is the registered per-connection `ServerTerminator`, and `GracefulTerminatorStage` only covers HTTP/1.1, so nothing else enforces the hard deadline. With the current `Duration.Inf` the binding's termination can hang for as long as a stream stays open. With a finite grace (issue 1) it still overruns whenever `hardDeadline` is shorter than the remaining grace. When already terminating, please (re)schedule `CompletionTimeout` if the new deadline is earlier than the one already scheduled, and add a directional test: age expires with a stream in flight, `terminate(short)` is called, and the connection closes within the short deadline. **3. The Java API cannot express or round-trip "infinite" (`javadsl/settings/Http2ServerSettings.scala`)** `getMaxConnectionAge` maps `Duration.Inf` to `ChronoUnit.FOREVER.getDuration` (via `JavaDurationConverter.toJava`), but `withMaxConnectionAge(java.time.Duration)` calls `age.toMillis`, which throws `ArithmeticException` for that value. So `settings.withMaxConnectionAge(settings.getMaxConnectionAge)` fails with the default settings, and Java users have no way to disable an age that was set in config. Please map `ChronoUnit.FOREVER.getDuration` back to `Duration.Inf` (or add an explicit way to disable it), and add a test covering the round trip. ### Minor (docs / comments) - `docs/.../http2.md` and `reference.conf`: please state that the setting only applies to HTTP/2 connections, and that HTTP/1.1 connections accepted on the same port are not age-limited. - `Http2ServerSpec`: the comment "with jitter disabled the connection is closed no earlier than the configured age" is stronger than the assertion, which only checks for no bytes during 400ms of a 500ms age. Either reword the comment or tighten the margin. -- 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]
