Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add client support for proxies that support HTTP CONNECT, such as squid. #148

Closed
wants to merge 4 commits into
from

Conversation

5 participants

coops commented Mar 22, 2013

No description provided.

@mariusae mariusae commented on an outdated diff Mar 25, 2013

...ala/com/twitter/finagle/http/HttpConnectHandler.scala
@@ -0,0 +1,109 @@
@mariusae

mariusae Mar 25, 2013

Contributor

Let's make this package name httpproxy so it doesn't collide with finagle-http.

Contributor

mariusae commented Mar 25, 2013

We shouldn't allow socksProxy and httpProxy to be defined simultaneously. Both orderings are valid (HTTP then SOCKS, or SOCKS then HTTP), but in practice it's probably an error if used in this way.

I think that, for now, a simple comment is sufficient: can you add something like the following to the builder options for both the SOCKS & HTTP proxies?

If this is defined concurrently with (http|socks)Proxy, the order in which they are applied is undefined.

Code review fixes.
- move HttpConnectHandler from com.twitter.finagle.http to com.twitter.finagle.httpproxy
- document undefined application order when both SOCKS and HTTP proxies are specified.
Contributor

mariusae commented Mar 25, 2013

Pulled internally; should appear here shortly!

@raofu raofu commented on the diff Mar 26, 2013

...om/twitter/finagle/httpproxy/HttpConnectHandler.scala
+ fail(ctx.getChannel, new InconsistentStateException(addr))
+ }
+ }
+
+ // we delay propagating connection upstream until we've completed the proxy connection.
+ override def channelConnected(ctx: ChannelHandlerContext, e: ChannelStateEvent) {
+ if (connectFuture.get eq null) {
+ fail(ctx.getChannel, new InconsistentStateException(addr))
+ return
+ }
+
+ // proxy cancellations again.
+ connectFuture.get.addListener(new ChannelFutureListener {
+ def operationComplete(f: ChannelFuture) {
+ if (f.isSuccess)
+ HttpConnectHandler.super.channelConnected(ctx, e)
@raofu

raofu Mar 26, 2013

Should this moved to messageReceived? I thank super.channelConnected should be called after the response for the HTTP CONNECT request is received.

@coops

coops Mar 26, 2013

i don't know netty well enough to say. if we change this behavior we should also change it in SocksConnectHandler.scala, which i cribbed it from.

coops commented Mar 29, 2013

sorry, I broke this by doing a small refactor and relying on the tests.

Contributor

mariusae commented Apr 1, 2013

Adding this internally now; thanks.

Contributor

sprsquish commented Apr 8, 2013

Committed: ca52719

@sprsquish sprsquish closed this Apr 8, 2013

coops commented May 14, 2013

hey guys, i don't think my bugfix commit made it in. it was foursquare/finagle@b77229e

Contributor

stevegury commented May 14, 2013

Your bugfix was integrated to the internal repo, we just need to synchronize the internal repo with the Github's one.
(It will be done this week)

coops commented May 14, 2013

cool, thanks. are you planning to cut a 6.3.1 at the same time?

Contributor

stevegury commented May 14, 2013

Yeah, It will be a 6.4.0 actually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment