-
Notifications
You must be signed in to change notification settings - Fork 240
Improve docs for http/connection-pool
's HTTP/2 support and ALPN config
#729
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Are you sure you want to mandate ALPN and update the docs, instead of updating the code? |
I think we should close the documentation PR and aim at updating the code with the snippet above. |
No I would prefer to update the code, as well. Will prepare a patch for that separately and once it's landed, we can get back to the doc update here.
The doc update is mostly orthogonal, only the wording about the ALPN config requirement would have to be changed once we've made it (more) optional again. |
5e71c68
to
cb5c03e
Compare
src/aleph/netty.clj
Outdated
Takes a vector of HTTP versions, in order of preference. E.g., `[:http2 :http1]` | ||
|
||
Note that the returned config uses `SelectorFailureBehavior.NO_ADVERTISE`[1] and | ||
`SelectedListenerFailureBehavior.ACCEPT`[2] since these are the only failure behaviors | ||
supported by all SSL providers. See their documentation for details. One important | ||
consequence of this is that it's not possible to completely opt out of HTTP/1.1 by way of | ||
only specifying `[:http2]`. | ||
|
||
1: https://netty.io/4.1/api/io/netty/handler/ssl/ApplicationProtocolConfig.SelectorFailureBehavior.html#NO_ADVERTISE | ||
2: https://netty.io/4.1/api/io/netty/handler/ssl/ApplicationProtocolConfig.SelectedListenerFailureBehavior.html#ACCEPT" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a line to the effect of: "If you want this behavior, you will need to supply an SslContext with a different config."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done via 439214b -- I made it about ApplicationProtocolConfig
instead since it can also be used with an ssl-context
options map, custom SslContext
isn't even required.
5caa131
to
7b562ae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, but see my minor doc suggestion
Prompted by #727