-
Notifications
You must be signed in to change notification settings - Fork 4k
Enable HTTP/2 Websocket in Web plugins by default #14500
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
base: main
Are you sure you want to change the base?
Conversation
num_conns_sups => get_env(num_conns_sup, 1) | ||
}, | ||
case cowboy:start_tls(RanchRef, RanchTransportOpts, CowboyOpts) of | ||
case cowboy:start_tls(RanchRef, RanchTransportOpts, CowboyOpts#{enable_connect_protocol => true}) of |
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.
This forces enable HTTP/2 Websocket. Not necessarily what we want. If enabled by default we want to be able to disable it.
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.
I assume the reasons to potentially not force HTTP/2 WebSockets has to do with client compatibility?
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.
Yes, especially because the rollout of HTTP/2 Websocket in browsers is not yet complete, so there may be unforeseen issues. It's also lacking real world testing in Cowboy.
This comment was marked as outdated.
This comment was marked as outdated.
1e35647
to
27d3aa7
Compare
I have only done manual testing of this so far. I would like to have tests in CI but as the feature freeze approaches I am focusing on finishing up the feature part of it. I will go back to adding tests once I have done the equivalent work for Web-AMQP10 in Tanzu. In the meantime this can be manually tested. |
This comment was marked as outdated.
This comment was marked as outdated.
This uses Cowboy's new direct data delivery mechanism, which provides more performance. Cowboy is now pinned to 2.14.0 and Cowlib to 2.16.0. HTTP/2 Websocket is enabled by default. It can be disabled as needed by setting `#{enable_connect_protocol => false}` in the plugin's `cowboy_opts`. Web-STOMP did not have HTTP/2 enabled before, now it does. Web-MQTT already had HTTP/2 enabled.
27d3aa7
to
725c2d5
Compare
This is ready. |
This PR:
Testing requires some configuration because the plugins don't enable TLS by default. I have made the following edit to make it work. Make sure to update the paths:
Details
I have been using Firefox to test and had to enable
network.http.http2.websockets
inabout:config
as it was disabled by default for me.