huw0 commented on code in PR #1454:
URL: https://github.com/apache/cxf/pull/1454#discussion_r1342159851
##########
rt/transports/http-jetty/src/main/java/org/apache/cxf/transport/http_jetty/JettyHTTPServerEngine.java:
##########
@@ -697,7 +697,8 @@ AbstractConnector
createConnectorJetty(SslContextFactory.Server sslcf, String ho
final SslConnectionFactory scf = new
SslConnectionFactory(sslcf, alpn.getProtocol());
connectionFactories.add(scf);
Review Comment:
As far as I can tell, the ordering of the ALPN connection factory doesn't
matter. From what I can see within Jetty's `AbstractConnector` the ordering of
the connection factories doesn't matter at the point where ALPN is started.
```
if (ssl != null)
{
String next = ssl.getNextProtocol();
ConnectionFactory cf = getConnectionFactory(next);
if (cf == null)
throw new IllegalStateException("No protocol factory for SSL
next protocol: '" + next + "' in " + this);
}
```
_In this case, `ssl.getNextProtocol()` will return ALPN_
However I am not that familiar with Jetty's internals and whether there is
anywhere else where this would matter.
The key thing is just that http2 precedes http/1.1 in the connection
factories list. There are four connection factories in total. The first of
these is on line 691 and is outside of the `isHttp2Enabled` block.
```
if (tlsServerParameters != null) {
connectionFactories.add(httpFactory);
httpConfig.addCustomizer(new
SecureRequestCustomizer(tlsServerParameters.isSniHostCheck()));
if (isHttp2Enabled(bus)) {
....
```
I would be happy to refactor so that the first connector (which is http/1.1)
is added after the `isHttp2Enabled` block, which would remove the need to
explicitly add http2 as the first item in the list.
##########
rt/transports/http-jetty/src/main/java/org/apache/cxf/transport/http_jetty/JettyHTTPServerEngine.java:
##########
@@ -697,7 +697,8 @@ AbstractConnector
createConnectorJetty(SslContextFactory.Server sslcf, String ho
final SslConnectionFactory scf = new
SslConnectionFactory(sslcf, alpn.getProtocol());
connectionFactories.add(scf);
Review Comment:
Thanks for looking over this change.
As far as I can tell, the ordering of the ALPN connection factory doesn't
matter. From what I can see within Jetty's `AbstractConnector` the ordering of
the connection factories doesn't matter at the point where ALPN is started.
```
if (ssl != null)
{
String next = ssl.getNextProtocol();
ConnectionFactory cf = getConnectionFactory(next);
if (cf == null)
throw new IllegalStateException("No protocol factory for SSL
next protocol: '" + next + "' in " + this);
}
```
_In this case, `ssl.getNextProtocol()` will return ALPN_
However I am not that familiar with Jetty's internals and whether there is
anywhere else where this would matter.
The key thing is just that http2 precedes http/1.1 in the connection
factories list. There are four connection factories in total. The first of
these is on line 691 and is outside of the `isHttp2Enabled` block.
```
if (tlsServerParameters != null) {
connectionFactories.add(httpFactory);
httpConfig.addCustomizer(new
SecureRequestCustomizer(tlsServerParameters.isSniHostCheck()));
if (isHttp2Enabled(bus)) {
....
```
I would be happy to refactor so that the first connector (which is http/1.1)
is added after the `isHttp2Enabled` block, which would remove the need to
explicitly add http2 as the first item in the list.
--
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]
