Skip to content

Commit

Permalink
Fixes #504 - HTTP/2 client transport cannot send request after idle t…
Browse files Browse the repository at this point in the history 
…imeout.

Made sure that the idle timeout mechanism notifies the destination
that the connection will close.

Also reviewed the close protocol to be: notify destination, then abort,
then close. In this way, HTTP/2 can send RST_STREAM before the
connection is closed.
  • Loading branch information
@sbordet
sbordet committed Apr 11, 2016
1 parent 11242ae commit f0a1ccf
Show file tree
Hide file tree
Showing 5 changed files with 157 additions and 23 deletions.
7 changes: 3 additions & 4 deletions jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpConnectionOverHTTP.java
View file
Expand Up @@ -138,17 +138,16 @@ protected void close(Throwable failure)
{
if (closed.compareAndSet(false, true))
{
// First close then abort, to be sure that the connection cannot be reused
// from an onFailure() handler or by blocking code waiting for completion.
getHttpDestination().close(this);

abort(failure);

getEndPoint().shutdownOutput();
if (LOG.isDebugEnabled())
LOG.debug("Shutdown {}", this);
getEndPoint().close();
if (LOG.isDebugEnabled())
LOG.debug("Closed {}", this);

abort(failure);
}
}

Expand Down
7 changes: 3 additions & 4 deletions .../fcgi-client/src/main/java/org/eclipse/jetty/fcgi/client/http/HttpConnectionOverFCGI.java
View file
Expand Up @@ -223,17 +223,16 @@ protected void close(Throwable failure)
{
if (closed.compareAndSet(false, true))
{
// First close then abort, to be sure that the connection cannot be reused
// from an onFailure() handler or by blocking code waiting for completion.
getHttpDestination().close(this);

abort(failure);

getEndPoint().shutdownOutput();
if (LOG.isDebugEnabled())
LOG.debug("Shutdown {}", this);
getEndPoint().close();
if (LOG.isDebugEnabled())
LOG.debug("Closed {}", this);

abort(failure);
}
}

Expand Down
25 changes: 20 additions & 5 deletions ...-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpConnectionOverHTTP2.java
View file
Expand Up @@ -20,6 +20,8 @@

import java.nio.channels.AsynchronousCloseException;
import java.util.Set;
import java.util.concurrent.TimeoutException;
import java.util.concurrent.atomic.AtomicBoolean;

import org.eclipse.jetty.client.HttpChannel;
import org.eclipse.jetty.client.HttpConnection;
Expand All @@ -35,6 +37,7 @@
public class HttpConnectionOverHTTP2 extends HttpConnection
{
private final Set<HttpChannel> channels = new ConcurrentHashSet<>();
private final AtomicBoolean closed = new AtomicBoolean();
private final Session session;

public HttpConnectionOverHTTP2(HttpDestination destination, Session session)
Expand Down Expand Up @@ -72,6 +75,15 @@ protected void release(HttpChannel channel)
getHttpDestination().release(this);
}

@Override
public boolean onIdleTimeout(long idleTimeout)
{
boolean close = super.onIdleTimeout(idleTimeout);
if (close)
close(new TimeoutException("idle_timeout"));
return false;
}

@Override
public void close()
{
Expand All @@ -80,11 +92,14 @@ public void close()

protected void close(Throwable failure)
{
// First close then abort, to be sure that the connection cannot be reused
// from an onFailure() handler or by blocking code waiting for completion.
getHttpDestination().close(this);
session.close(ErrorCode.NO_ERROR.code, failure.getMessage(), Callback.NOOP);
abort(failure);
if (closed.compareAndSet(false, true))
{
getHttpDestination().close(this);

abort(failure);

session.close(ErrorCode.NO_ERROR.code, failure.getMessage(), Callback.NOOP);
}
}

private void abort(Throwable failure)
Expand Down
77 changes: 77 additions & 0 deletions ...t/src/test/java/org/eclipse/jetty/http2/client/http/HttpClientTransportOverHTTP2Test.java
View file
Expand Up @@ -313,6 +313,83 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques
Assert.assertEquals(HttpStatus.OK_200, response.getStatus());
}

@Test
public void testConnectionIdleTimeoutSendsResetFrame() throws Exception
{
long idleTimeout = 1000;

CountDownLatch resetLatch = new CountDownLatch(1);
start(new ServerSessionListener.Adapter()
{
@Override
public Stream.Listener onNewStream(Stream stream, HeadersFrame frame)
{
return new Stream.Listener.Adapter()
{
@Override
public void onReset(Stream stream, ResetFrame frame)
{
resetLatch.countDown();
}
};
}
});
client.stop();
client.setIdleTimeout(idleTimeout);
client.start();

try
{
client.newRequest("localhost", connector.getLocalPort())
// Make sure the connection idle times out, not the stream.
.idleTimeout(2 * idleTimeout, TimeUnit.MILLISECONDS)
.send();
Assert.fail();
}
catch (ExecutionException e)
{
// Expected.
}

Assert.assertTrue(resetLatch.await(5, TimeUnit.SECONDS));
}

@Test
public void testRequestIdleTimeoutSendsResetFrame() throws Exception
{
CountDownLatch resetLatch = new CountDownLatch(1);
start(new ServerSessionListener.Adapter()
{
@Override
public Stream.Listener onNewStream(Stream stream, HeadersFrame frame)
{
return new Stream.Listener.Adapter()
{
@Override
public void onReset(Stream stream, ResetFrame frame)
{
resetLatch.countDown();
}
};
}
});

try
{
long idleTimeout = 1000;
client.newRequest("localhost", connector.getLocalPort())
.idleTimeout(idleTimeout, TimeUnit.MILLISECONDS)
.send();
Assert.fail();
}
catch (ExecutionException e)
{
// Expected.
}

Assert.assertTrue(resetLatch.await(5, TimeUnit.SECONDS));
}

@Ignore
@Test
public void testExternalServer() throws Exception
Expand Down
64 changes: 54 additions & 10 deletions ...ient-transport/src/test/java/org/eclipse/jetty/http/client/HttpClientIdleTimeoutTest.java
View file
Expand Up @@ -52,22 +52,31 @@ public void testClientIdleTimeout() throws Exception
public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException
{
baseRequest.setHandled(true);
AsyncContext asyncContext = request.startAsync();
asyncContext.setTimeout(0);
if (target.equals("/timeout"))
{
AsyncContext asyncContext = request.startAsync();
asyncContext.setTimeout(0);
}
}
});
client.stop();
client.setIdleTimeout(idleTimeout);
client.start();

final CountDownLatch latch = new CountDownLatch(1);
client.newRequest(newURI()).send(result ->
{
if (result.isFailed())
latch.countDown();
});
client.newRequest(newURI())
.path("/timeout")
.send(result ->
{
if (result.isFailed())
latch.countDown();
});

Assert.assertTrue(latch.await(2 * idleTimeout, TimeUnit.MILLISECONDS));

// Verify that after the timeout we can make another request.
ContentResponse response = client.newRequest(newURI()).send();
Assert.assertEquals(HttpStatus.OK_200, response.getStatus());
}

@Test
Expand All @@ -79,13 +88,17 @@ public void testRequestIdleTimeout() throws Exception
public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException
{
baseRequest.setHandled(true);
AsyncContext asyncContext = request.startAsync();
asyncContext.setTimeout(0);
if (target.equals("/timeout"))
{
AsyncContext asyncContext = request.startAsync();
asyncContext.setTimeout(0);
}
}
});

final CountDownLatch latch = new CountDownLatch(1);
client.newRequest(newURI())
.path("/timeout")
.idleTimeout(idleTimeout, TimeUnit.MILLISECONDS)
.send(result ->
{
Expand All @@ -94,10 +107,41 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques
});

Assert.assertTrue(latch.await(2 * idleTimeout, TimeUnit.MILLISECONDS));

// Verify that after the timeout we can make another request.
ContentResponse response = client.newRequest(newURI()).send();
Assert.assertEquals(HttpStatus.OK_200, response.getStatus());
}

@Test
public void testIdleClientIdleTimeout() throws Exception
{
start(new AbstractHandler()
{
@Override
public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException
{
baseRequest.setHandled(true);
}
});
client.stop();
client.setIdleTimeout(idleTimeout);
client.start();

// Make a first request to open a connection.
ContentResponse response = client.newRequest(newURI()).send();
Assert.assertEquals(HttpStatus.OK_200, response.getStatus());

// Let the connection idle timeout.
Thread.sleep(2 * idleTimeout);

// Verify that after the timeout we can make another request.
response = client.newRequest(newURI()).send();
Assert.assertEquals(HttpStatus.OK_200, response.getStatus());
}

@Test
public void testServerIdleTimeout() throws Exception
public void testIdleServerIdleTimeout() throws Exception
{
start(new AbstractHandler()
{
Expand Down

0 comments on commit f0a1ccf

Please sign in to comment.