Bug 360665

Comment 1 Chris Dumoulin 2011-10-12 09:45:08 EDT

Created attachment 205037 [details]
Test program to reproduce the problem.

Comment 2 Thomas Becker 2011-10-13 05:28:48 EDT

Created attachment 205109 [details]
Unit Test

Hi Chris,

I've written a pure jetty unit test which is doing what leads to your exception loop.

- create a jetty server on a random port acting as the proxy
- start a client using that proxy
- send an https request (e.g. I tried https://google.com, https://web.de, etc.). 

Test is always green and the response is as expected. Could you please confirm that you get this problem also when the proxy is running on a high port (e.g. 8080) and with a more recent jetty version?

Attached is the unit test. Feel free to modify it to reproduce your issue.

Cheers,
Thomas

Comment 3 Thomas Becker 2011-10-13 05:29:39 EDT

Oh and what kind of proxy are you running on port 80?

Comment 4 Thomas Becker 2011-10-13 05:42:34 EDT

For the record: 

I've had a look at the code from jetty 7.2.0 and the exception is thrown if a non printing char occurs in the parsed string:

                    else if (ch < HttpTokens.SPACE && ch>=0)
                    {
                        throw new HttpException(HttpStatus.BAD_REQUEST_400);
                    }

I can't see an obvious reason how this can happen in your case. So it'll be good to get the information I requested above.

Cheers,
Thomas

Comment 5 Chris Dumoulin 2011-10-13 08:45:28 EDT

(In reply to comment #3)
> Oh and what kind of proxy are you running on port 80?

Nginx was running on port 80. Using wireshark I can see that the HttpClient sends an HTTP CONNECT request, which I've read nginx doesn't support. Nginx sends back a malformed response; it contains some HTML but no HTTP headers.

I'm not concerned that an exception is thrown; the problem is the fact that it loops continuously once this happens.

I'll try running the unit test that was attached.

Comment 6 Chris Dumoulin 2011-10-13 09:06:25 EDT

I haven't been able to run the unit test, but I doubt it would show the problem. I'm pretty sure you need a web server that doesn't handle the HTTP CONNECT request.

I initially saw the problem using a site mirroring tool called netspiegel (https://github.com/ariya/X2/tree/d6fac7ebe456956854bd17e9405d4e873fd1b4f7/network/netspiegel). This program can act as a web server, and simply drops the connection if it gets an HTTP request whose method isn't GET.

Using wireshark I'm able to see the HTTP CONNECT being sent, and then the connection is broken. The exception I see when using netspiegel is:

2011-10-13 08:55:26.751:WARN::EXCEPTION ConnectExchange@6131844=CONNECT//ec2-67-202-16-167.compute-1.amazonaws.com:80google.com:443#9
org.eclipse.jetty.io.EofException
	at org.eclipse.jetty.http.HttpParser.parseNext(HttpParser.java:319)
	at org.eclipse.jetty.http.HttpParser.parseAvailable(HttpParser.java:212)
	at org.eclipse.jetty.client.HttpConnection.handle(HttpConnection.java:262)
	at org.eclipse.jetty.io.nio.SelectChannelEndPoint.handle(SelectChannelEndPoint.java:510)
	at org.eclipse.jetty.io.nio.SelectChannelEndPoint.access$000(SelectChannelEndPoint.java:34)
	at org.eclipse.jetty.io.nio.SelectChannelEndPoint$1.run(SelectChannelEndPoint.java:40)
	at org.eclipse.jetty.util.thread.QueuedThreadPool$2.run(QueuedThreadPool.java:450)
	at java.lang.Thread.run(Thread.java:662)

Comment 7 Thomas Becker 2011-10-14 10:00:23 EDT

Are you 100% sure that the client is not rerequesting and that jetty loops with a single request being made by the client?

Can you attach a snoop file for a single request which I can examine in wireshark?

Comment 8 Chris Dumoulin 2011-10-14 10:25:54 EDT

(In reply to comment #7)
> Are you 100% sure that the client is not rerequesting and that jetty loops with
> a single request being made by the client?
> 
> Can you attach a snoop file for a single request which I can examine in
> wireshark?

There are repeated HTTP CONNECT requests made, but only one call to HttpClient.send(). I'm using BadProxyTest.java which only does one send() and has no looping.

I'm attaching a traffic dump taken from wireshark.

Comment 9 Chris Dumoulin 2011-10-14 10:27:40 EDT

Created attachment 205200 [details]
TCP data recorded using wireshark

Comment 10 Thomas Becker 2011-10-31 08:59:05 EDT

Created attachment 206205 [details]
Test to reproduce the issue.

Given the new information I've been able to reproduce the issue with a Unit test using plain jetty. 

The client ends up in the loop when the proxy responsible for handling the Connect request will simply close the connection without sending any response (as shown in your wireshark).

I will now find and fix the root cause. :)

Comment 11 Thomas Becker 2011-10-31 15:13:39 EDT

Created attachment 206236 [details]
proposed patch

Proposed patch to remove the HttpExchange from HttpClients queue in HttpDestination.ConnectExchange.onException().

Comment 12 Thomas Becker 2011-11-02 09:06:13 EDT

Created attachment 206323 [details]
proposed patch

Better patch attached + Tests.

Comment 13 Thomas Becker 2011-11-03 06:30:54 EDT

Created attachment 206394 [details]
proposed patch

New patch files. Now with support for onExpire and little changes in onResponseComplete as discussed.

Comment 14 Thomas Becker 2011-11-03 10:01:46 EDT

Created attachment 206406 [details]
proposed patch

Additional commit for a new testcase (504 returned by proxy) and enhanced unit tests by reusing the proxy.

Comment 15 Thomas Becker 2011-11-03 12:45:33 EDT

Created attachment 206417 [details]
proposed patch

Merged the 5 commits to a single one as requested.

Comment 16 Simone Bordet 2011-11-04 10:51:39 EDT

Patch applied with some additional tweaks.

Comment 17 Simone Bordet 2011-11-04 10:52:26 EDT

Fixed.