Skip to content

Commit

Permalink
Fixes #7348 - Slow CONNECT request causes NPE
Browse files Browse the repository at this point in the history
Fixed `HttpClientTest.testCONNECTWithHTTP10()` logic
after changes to fix this issue.

Now a tunneled connection is not put back into the connection pool,
and if applications explicitly want to use it, they must re-enable
fill interest, similarly to what should be done after upgrade+101.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
  • Loading branch information
sbordet committed Jan 3, 2022
1 parent f1f1c3a commit ad125ae
Show file tree
Hide file tree
Showing 4 changed files with 22 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ public void exchangeTerminated(HttpExchange exchange, Result result)
{
super.exchangeTerminated(exchange, result);

String method = exchange.getRequest().getMethod();
Response response = result.getResponse();
HttpFields responseHeaders = response.getHeaders();

Expand All @@ -154,7 +155,7 @@ else if (sender.isShutdown())
// HTTP 1.0 must close the connection unless it has
// an explicit keep alive or it's a CONNECT method.
boolean keepAlive = responseHeaders.contains(HttpHeader.CONNECTION, HttpHeaderValue.KEEP_ALIVE.asString());
boolean connect = HttpMethod.CONNECT.is(exchange.getRequest().getMethod());
boolean connect = HttpMethod.CONNECT.is(method);
if (!keepAlive && !connect)
closeReason = "http/1.0";
}
Expand All @@ -174,7 +175,8 @@ else if (sender.isShutdown())
}
else
{
if (response.getStatus() == HttpStatus.SWITCHING_PROTOCOLS_101)
int status = response.getStatus();
if (status == HttpStatus.SWITCHING_PROTOCOLS_101 || isTunnel(method, status))
connection.remove();
else
release();
Expand All @@ -191,6 +193,11 @@ protected long getMessagesOut()
return outMessages.longValue();
}

boolean isTunnel(String method, int status)
{
return HttpMethod.CONNECT.is(method) && HttpStatus.isSuccess(status);
}

@Override
public String toString()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ private boolean parse()
// Connection upgrade due to CONNECT + 200, bail out.
String method = this.method;
this.method = null;
if (status == HttpStatus.OK_200 && HttpMethod.CONNECT.is(method))
if (getHttpChannel().isTunnel(method, status))
return true;
}

Expand Down Expand Up @@ -291,8 +291,7 @@ public boolean startResponse(HttpVersion version, int status, String reason)

this.method = exchange.getRequest().getMethod();
this.status = status;
parser.setHeadResponse(HttpMethod.HEAD.is(method) ||
(HttpMethod.CONNECT.is(method) && status == HttpStatus.OK_200));
parser.setHeadResponse(HttpMethod.HEAD.is(method) || getHttpChannel().isTunnel(method, status));
exchange.getResponse().version(version).status(status).reason(reason);

return !responseBegin(exchange);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1606,8 +1606,18 @@ public void testCONNECTWithHTTP10(Scenario scenario) throws Exception

ContentResponse response = listener.get(5, TimeUnit.SECONDS);
assertEquals(200, response.getStatus());
assertThat(connection, Matchers.instanceOf(HttpConnectionOverHTTP.class));
HttpConnectionOverHTTP httpConnection = (HttpConnectionOverHTTP)connection;
EndPoint endPoint = httpConnection.getEndPoint();
assertTrue(endPoint.isOpen());

// After a CONNECT+200, this connection is in "tunnel mode",
// and applications that want to deal with tunnel bytes must
// likely access the underlying EndPoint.
// For the purpose of this test, we just re-enable fill interest
// so that we can send another clear-text HTTP request.
httpConnection.fillInterested();

// Test that I can send another request on the same connection.
request = client.newRequest(host, port);
listener = new FutureResponseListener(request);
connection.send(request, listener);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -807,7 +807,6 @@ public void testRequestCompletionDelayed(SslContextFactory.Server proxyTLS) thro
@Override
public void onSuccess(org.eclipse.jetty.client.api.Request request)
{
System.err.println("SIMON: request = " + request);
if (HttpMethod.CONNECT.is(request.getMethod()))
sleep(250);
}
Expand Down

0 comments on commit ad125ae

Please sign in to comment.