pjfanning opened a new pull request, #1232:
URL: https://github.com/apache/pekko-http/pull/1232
### Motivation
`Http2AlpnSupport` was a thin facade in front of `Http2JDKAlpnSupport`,
split in two purely so the ALPN classes would load lazily. The documentation
still describes a world that no longer exists:
> Will add support to an engine either using jetty alpn or using netty APIs
(later).
> The actual implementation of ALPN support on supported JDKs. We rely on
lazy class loading to not fail with class loading errors when ALPN support is
missing.
with a matching comment in `Http2.scala`:
```scala
def getChosenProtocol(): String =
chosenProtocol.getOrElse(Http2AlpnSupport.HTTP11) // default to http/1, e.g.
when ALPN jar is missing
```
All of that dates from Java 8, where ALPN needed an external jar (jetty-alpn
or similar). This branch requires JDK 17, where ALPN is part of
`javax.net.ssl`, so neither the split nor the lazy-loading story serves any
purpose.
While reading it I found something else. `clientSetApplicationProtocols`
**ignores its `protocols` parameter**:
```scala
def clientSetApplicationProtocols(engine: SSLEngine, protocols:
Array[String]): Unit = {
val params = engine.getSSLParameters
params.setApplicationProtocols(Array("h2")) // protocols is never read
engine.setSSLParameters(params)
}
```
Invisible today, because the sole caller (`Http2.scala:261`) passes exactly
`Array("h2")` - but a trap for anyone who reuses it.
### Modification
- Merge the two objects into `Http2AlpnSupport` and make `chooseProtocol`
private to it.
- Pass `protocols` through to `setApplicationProtocols` rather than ignoring
it. No behaviour change, since the only call site already passes `Array("h2")`.
- Correct the scaladoc and the `Http2.scala` comment.
- Drop `applySessionParameters`, a one-line delegate to
`TlsUtils.applySessionParameters` with no callers anywhere. Flagging it
explicitly in case its removal is unwelcome - it is `private[http]`, so nothing
outside pekko-http could have been using it.
- The inner lambda shadowed the outer `engine`; it is now `_`, since it was
unused.
Both objects are `@InternalApi` / `private[http]`. Removing
`Http2JDKAlpnSupport` is a `MissingClassProblem`, so this adds
`http-core/src/main/mima-filters/2.0.x.backwards.excludes/remove-http2jdkalpnsupport.excludes`,
in the same shape as the existing `remove-bytestringinputstream.excludes` and
`remove-previewserversettings.excludes` entries for other removed `impl`
classes. Calling that out rather than burying it: it is a deliberate filter
addition, matching established practice in this repo for internal-class
removals, not a suppressed warning.
### Result
One object instead of two, documentation that describes what the code
actually does, and no silently ignored parameter.
### Tests
- `sbt "http2-tests / Test / testOnly
org.apache.pekko.http.impl.engine.http2.Http2ClientServerSpec
org.apache.pekko.http.impl.engine.http2.ProtocolSwitchSpec"` - 10 passed, 1
ignored (pre-existing). These negotiate h2 over TLS end to end, so they
exercise both `enableForServer` and `clientSetApplicationProtocols`, plus the
http1 fallback path through `getChosenProtocol`.
- `sbt http-core/mimaReportBinaryIssues` - clean with the filter; without
it, reports exactly the two expected `MissingClassProblem`s for
`Http2JDKAlpnSupport` and `Http2JDKAlpnSupport$`.
- `scalafmt --mode diff-ref=upstream/main` - clean.
No new test: this is a refactor with no behaviour change, and the ALPN
negotiation path it touches is already covered end to end by the specs above.
The one semantic change - honouring `protocols` - cannot be observed without a
second caller passing something other than `Array("h2")`.
### References
None - found while reviewing the code base against the JDK 17 baseline
--
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]
