Skip to content

Conversation

jroper
Copy link
Contributor

@jroper jroper commented Jul 30, 2014

The WS standard requires that clients use the CONNECT method when proxying WebSockets, even if they aren't using SSL. The Grizzly provider already supported this, but the Netty didn't.

I also implemented a test that tested this case.

I've made this change and PR against the 1.8.x branch, master is significantly different but briefly reading the code it looks like the bug is there as well. If you'd prefer it against master first, and then backported, or want me to submit another PR against master/1.9.x, let me know.

jroper added 2 commits July 30, 2014 10:11
The WS standard requires that clients use the CONNECT method when
proxying WebSockets, even if they aren't using SSL.  The Grizzly
provider already supported this, but the Netty didn't.

I also implemented a test that tested this case.
This needed to be done both for the Grizzly and the Netty provider.

Since this is essentially the same configuration parameter as the
useRelativeURIsWithSSLProxies, I reused that parameter, but I renamed it
to useRelativeURIsWithConnectProxies to reflect its true meaning since
it's no longer used just for SSL, and I deprecated the old parameter.
@jroper
Copy link
Contributor Author

jroper commented Jul 30, 2014

I've attached a second commit to this pull request, it ensures that when proxying websockets using the CONNECT method, a relative path is used. This slightly changed the meaning of the useRelativeURIsWithSSLProxies configuration parameter, so I renamed it to reflect it's new meaning, useRelativeURIsWithConnectProxies, and deprecated the old parameter.

Let me know if you'd prefer these two commits to be squashed.

@slandelle
Copy link
Contributor

@jroper Thanks! Could you squash the commits please? I'll then port on 1.9 and master.

slandelle pushed a commit that referenced this pull request Jul 30, 2014
Ensured Netty provider uses CONNECT when proxying ws
@slandelle slandelle merged commit 237ba25 into AsyncHttpClient:1.8.x Jul 30, 2014
@slandelle slandelle added this to the 1.8.13 milestone Jul 30, 2014
@slandelle slandelle self-assigned this Jul 30, 2014
@jroper
Copy link
Contributor Author

jroper commented Jul 30, 2014

I guess you don't want me to squash the commits anymore?

@slandelle
Copy link
Contributor

Nope, thanks :) I thought you were offline at this time and I wanted to port this ASAP on the other branches.

@jroper jroper deleted the websocket-proxy-connect branch July 30, 2014 08:13
slandelle pushed a commit that referenced this pull request Jul 30, 2014
@slandelle
Copy link
Contributor

@jroper I guess you need me to release 1.8.13?

@jroper
Copy link
Contributor Author

jroper commented Jul 30, 2014

No rush, we fixed this for a customer but that customer can use their own builds until you release it.

@muymoo
Copy link

muymoo commented Jul 31, 2014

That customer is very happy. Tested in a corporate environment and ws/wss worked for me. Thanks!

@slandelle
Copy link
Contributor

Thanks for your feedback, and thanks again to @jroper for the fix!

cs-workco pushed a commit to cs-workco/async-http-client that referenced this pull request Apr 13, 2023
* Fix HTTP2StreamChannel leak

* Update code comments.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants