From 41050cd8a438e24f7f8a3e07a454c74f084ab792 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Thu, 4 Jan 2018 13:26:31 +0100 Subject: [PATCH] Issue #2081 No idle timeout exception when dispatch is delayed (#2083) Issue #2081 No idle timeout exception when dispatch is delayed * Delegate the readtimeout handling to HttpChannel so that a delayed dispatch can be ended. * Added unit test for delayed dispatch idle * Now using HttpInput.onIdleTimeout() to fail the HttpInput, and then dispatching the request in case it has not been dispatched yet. This ensure consistent behavior independently of the value of HttpConfiguration.delayDispatchUntilContent. * Fixed for both HTTP/1.1 and HTTP/2. * Added tests for non-blocking reads. Signed-off-by: Greg Wilkins Signed-off-by: Simone Bordet --- .../jetty/client/HttpClientTimeoutTest.java | 4 +- .../jetty/client/ssl/SslBytesServerTest.java | 4 +- .../http2/server/HttpChannelOverHTTP2.java | 15 ++- .../eclipse/jetty/io/AbstractConnection.java | 6 +- .../jetty/server/HttpChannelOverHttp.java | 11 ++ .../eclipse/jetty/server/HttpConnection.java | 8 +- .../org/eclipse/jetty/server/HttpInput.java | 3 +- .../jetty/server/ConnectorTimeoutTest.java | 90 ++++++++++++- .../websocket/client/ClientCloseTest.java | 21 ++-- .../io/AbstractWebSocketConnection.java | 8 +- .../jetty/http/client/ServerTimeoutsTest.java | 119 +++++++++++++++--- 11 files changed, 236 insertions(+), 53 deletions(-) diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTimeoutTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTimeoutTest.java index 639d9af7bac..851118c3c95 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTimeoutTest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTimeoutTest.java @@ -263,10 +263,10 @@ public class HttpClientTimeoutTest extends AbstractHttpClientServerTest return new SslConnection(byteBufferPool, executor, endPoint, engine) { @Override - protected boolean onReadTimeout() + protected boolean onReadTimeout(Throwable timeout) { sslIdle.set(true); - return super.onReadTimeout(); + return super.onReadTimeout(timeout); } }; } diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/ssl/SslBytesServerTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/ssl/SslBytesServerTest.java index 735d08b4299..db32212ee55 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/ssl/SslBytesServerTest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/ssl/SslBytesServerTest.java @@ -126,12 +126,12 @@ public class SslBytesServerTest extends SslBytesTest } @Override - protected boolean onReadTimeout() + protected boolean onReadTimeout(Throwable timeout) { final Runnable idleHook = SslBytesServerTest.this.idleHook; if (idleHook != null) idleHook.run(); - return super.onReadTimeout(); + return super.onReadTimeout(timeout); } }, connector, endPoint); } diff --git a/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpChannelOverHTTP2.java b/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpChannelOverHTTP2.java index 484308bf033..4d8c34d1565 100644 --- a/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpChannelOverHTTP2.java +++ b/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpChannelOverHTTP2.java @@ -315,16 +315,19 @@ public class HttpChannelOverHTTP2 extends HttpChannel implements Closeable, Writ public boolean onStreamTimeout(Throwable failure, Consumer consumer) { - boolean result = false; - if (isRequestIdle()) - { + boolean delayed = _delayedUntilContent; + _delayedUntilContent = false; + + boolean result = isRequestIdle(); + if (result) consumeInput(); - result = true; - } getHttpTransport().onStreamTimeout(failure); - if (getRequest().getHttpInput().onIdleTimeout(failure)) + if (getRequest().getHttpInput().onIdleTimeout(failure) || delayed) + { consumer.accept(this::handleWithContext); + result = false; + } return result; } diff --git a/jetty-io/src/main/java/org/eclipse/jetty/io/AbstractConnection.java b/jetty-io/src/main/java/org/eclipse/jetty/io/AbstractConnection.java index 7cbadbfb6ee..5dd0ff980e1 100644 --- a/jetty-io/src/main/java/org/eclipse/jetty/io/AbstractConnection.java +++ b/jetty-io/src/main/java/org/eclipse/jetty/io/AbstractConnection.java @@ -167,7 +167,7 @@ public abstract class AbstractConnection implements Connection { boolean close = true; if (cause instanceof TimeoutException) - close = onReadTimeout(); + close = onReadTimeout(cause); if (close) { if (_endPoint.isOutputShutdown()) @@ -183,9 +183,11 @@ public abstract class AbstractConnection implements Connection /** *

Callback method invoked when the endpoint failed to be ready to be read after a timeout

+ * + * @param timeout the cause of the read timeout * @return true to signal that the endpoint must be closed, false to keep the endpoint open */ - protected boolean onReadTimeout() + protected boolean onReadTimeout(Throwable timeout) { return true; } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelOverHttp.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelOverHttp.java index 54dd7139615..c4ed4d420ae 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelOverHttp.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelOverHttp.java @@ -399,6 +399,17 @@ public class HttpChannelOverHttp extends HttpChannel implements HttpParser.Reque return !_delayedForContent; } + boolean onIdleTimeout(Throwable timeout) + { + if (_delayedForContent) + { + _delayedForContent = false; + getRequest().getHttpInput().onIdleTimeout(timeout); + execute(this); + return false; + } + return true; + } /** *

Attempts to perform a HTTP/1.1 upgrade.

diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java index f1d11f2db66..3f0bb134c90 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java @@ -286,7 +286,7 @@ public class HttpConnection extends AbstractConnection implements Runnable, Http LOG.debug("{} onFillable exit {} {}", this, _channel.getState(),BufferUtil.toDetailString(_requestBuffer)); } } - + /* ------------------------------------------------------------ */ /** Fill and parse data looking for content * @return true if an {@link RequestHandler} method was called and it returned true; @@ -486,6 +486,12 @@ public class HttpConnection extends AbstractConnection implements Runnable, Http } } + @Override + protected boolean onReadTimeout(Throwable timeout) + { + return _channel.onIdleTimeout(timeout); + } + @Override protected void onFillInterestedFailed(Throwable cause) { diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpInput.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpInput.java index 504ab78d062..4519710920a 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpInput.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpInput.java @@ -782,7 +782,8 @@ public class HttpInput extends ServletInputStream implements Runnable { synchronized (_inputQ) { - if (_waitingForContent && !isError()) + boolean neverDispatched = getHttpChannelState().isIdle(); + if ((_waitingForContent || neverDispatched) && !isError()) { x.addSuppressed(new Throwable("HttpInput idle timeout")); _state = new ErrorState(x); diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/ConnectorTimeoutTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/ConnectorTimeoutTest.java index 024e35c8074..e83875b4401 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/ConnectorTimeoutTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/ConnectorTimeoutTest.java @@ -601,7 +601,8 @@ public abstract class ConnectorTimeoutTest extends HttpServerTestFixture long start = System.currentTimeMillis(); try { - IO.toString(is); + String response = IO.toString(is); + Assert.assertThat(response,Matchers.is("")); Assert.assertEquals(-1, is.read()); } catch(SSLException e) @@ -629,12 +630,13 @@ public abstract class ConnectorTimeoutTest extends HttpServerTestFixture long start = System.currentTimeMillis(); try { - IO.toString(is); + String response = IO.toString(is); + Assert.assertThat(response,Matchers.is("")); Assert.assertEquals(-1, is.read()); } catch(SSLException e) { - // e.printStackTrace(); + e.printStackTrace(); } catch(Exception e) { @@ -644,6 +646,88 @@ public abstract class ConnectorTimeoutTest extends HttpServerTestFixture } + @Test(timeout=60000) + public void testMaxIdleDelayedDispatch() throws Exception + { + configureServer(new EchoHandler()); + Socket client=newSocket(_serverURI.getHost(),_serverURI.getPort()); + client.setSoTimeout(10000); + InputStream is=client.getInputStream(); + Assert.assertFalse(client.isClosed()); + + OutputStream os=client.getOutputStream(); + os.write(( + "GET / HTTP/1.1\r\n"+ + "host: "+_serverURI.getHost()+":"+_serverURI.getPort()+"\r\n"+ + "connection: keep-alive\r\n"+ + "Content-Length: 20\r\n"+ + "Content-Type: text/plain\r\n"+ + "Connection: close\r\n"+ + "\r\n").getBytes("utf-8")); + os.flush(); + + long start = System.currentTimeMillis(); + try + { + String response = IO.toString(is); + Assert.assertThat(response,Matchers.containsString("500")); + Assert.assertEquals(-1, is.read()); + } + catch(SSLException e) + { + e.printStackTrace(); + } + catch(Exception e) + { + e.printStackTrace(); + } + int duration = (int)(System.currentTimeMillis() - start); + Assert.assertThat(duration,Matchers.greaterThanOrEqualTo(MAX_IDLE_TIME)); + Assert.assertThat(duration,Matchers.lessThan(maximumTestRuntime)); + } + + @Test(timeout=60000) + public void testMaxIdleDispatch() throws Exception + { + configureServer(new EchoHandler()); + Socket client=newSocket(_serverURI.getHost(),_serverURI.getPort()); + client.setSoTimeout(10000); + InputStream is=client.getInputStream(); + Assert.assertFalse(client.isClosed()); + + OutputStream os=client.getOutputStream(); + os.write(( + "GET / HTTP/1.1\r\n"+ + "host: "+_serverURI.getHost()+":"+_serverURI.getPort()+"\r\n"+ + "connection: keep-alive\r\n"+ + "Content-Length: 20\r\n"+ + "Content-Type: text/plain\r\n"+ + "Connection: close\r\n"+ + "\r\n"+ + "1234567890").getBytes("utf-8")); + os.flush(); + + long start = System.currentTimeMillis(); + try + { + String response = IO.toString(is); + Assert.assertThat(response,Matchers.containsString("500")); + Assert.assertEquals(-1, is.read()); + } + catch(SSLException e) + { + e.printStackTrace(); + } + catch(Exception e) + { + e.printStackTrace(); + } + int duration = (int)(System.currentTimeMillis() - start); + Assert.assertThat(duration,Matchers.greaterThanOrEqualTo(MAX_IDLE_TIME)); + Assert.assertThat(duration,Matchers.lessThan(maximumTestRuntime)); + } + + @Test(timeout=60000) public void testMaxIdleWithSlowRequest() throws Exception { diff --git a/jetty-websocket/websocket-client/src/test/java/org/eclipse/jetty/websocket/client/ClientCloseTest.java b/jetty-websocket/websocket-client/src/test/java/org/eclipse/jetty/websocket/client/ClientCloseTest.java index c5a9823df24..ed357a3457c 100644 --- a/jetty-websocket/websocket-client/src/test/java/org/eclipse/jetty/websocket/client/ClientCloseTest.java +++ b/jetty-websocket/websocket-client/src/test/java/org/eclipse/jetty/websocket/client/ClientCloseTest.java @@ -18,14 +18,6 @@ package org.eclipse.jetty.websocket.client; -import static org.hamcrest.Matchers.allOf; -import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.instanceOf; -import static org.hamcrest.Matchers.is; -import static org.hamcrest.Matchers.notNullValue; -import static org.hamcrest.Matchers.nullValue; -import static org.junit.Assert.assertThat; - import java.io.IOException; import java.lang.reflect.Field; import java.net.SocketTimeoutException; @@ -78,6 +70,14 @@ import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; +import static org.hamcrest.Matchers.allOf; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.instanceOf; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.notNullValue; +import static org.hamcrest.Matchers.nullValue; +import static org.junit.Assert.assertThat; + public class ClientCloseTest { private static final Logger LOG = Log.getLogger(ClientCloseTest.class); @@ -147,7 +147,7 @@ public class ClientCloseTest @Override public void onWebSocketError(Throwable cause) { - LOG.warn("onWebSocketError",cause); + LOG.debug("onWebSocketError",cause); assertThat("Unique Error Event", error.compareAndSet(null, cause), is(true)); errorLatch.countDown(); } @@ -526,8 +526,7 @@ public class ClientCloseTest // client idle timeout triggers close event on client ws-endpoint assertThat("OnError Latch", clientSocket.errorLatch.await(2, TimeUnit.SECONDS), is(true)); - assertThat("OnError", clientSocket.error.get(), instanceOf(SocketTimeoutException.class)); - assertThat("OnError", clientSocket.error.get().getMessage(), containsString("Timeout on Read")); + assertThat("OnError", clientSocket.error.get(), instanceOf(TimeoutException.class)); } @Test(timeout = 5000L) diff --git a/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/io/AbstractWebSocketConnection.java b/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/io/AbstractWebSocketConnection.java index c32c7367aa4..32747e1166e 100644 --- a/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/io/AbstractWebSocketConnection.java +++ b/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/io/AbstractWebSocketConnection.java @@ -21,7 +21,6 @@ package org.eclipse.jetty.websocket.common.io; import java.io.EOFException; import java.io.IOException; import java.net.InetSocketAddress; -import java.net.SocketTimeoutException; import java.nio.ByteBuffer; import java.util.ArrayList; import java.util.List; @@ -448,11 +447,8 @@ public abstract class AbstractWebSocketConnection extends AbstractConnection imp this.ioState.onOpened(); } - /** - * Event for no activity on connection (read or write) - */ @Override - protected boolean onReadTimeout() + protected boolean onReadTimeout(Throwable timeout) { IOState state = getIOState(); ConnectionState cstate = state.getConnectionState(); @@ -470,7 +466,7 @@ public abstract class AbstractWebSocketConnection extends AbstractConnection imp try { - notifyError(new SocketTimeoutException("Timeout on Read")); + notifyError(timeout); } finally { diff --git a/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/ServerTimeoutsTest.java b/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/ServerTimeoutsTest.java index 7acd68c5a95..795a5930c59 100644 --- a/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/ServerTimeoutsTest.java +++ b/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/ServerTimeoutsTest.java @@ -46,6 +46,7 @@ import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http2.FlowControlStrategy; import org.eclipse.jetty.http2.client.http.HttpClientTransportOverHTTP2; import org.eclipse.jetty.http2.server.AbstractHTTP2ServerConnectionFactory; +import org.eclipse.jetty.server.Handler; import org.eclipse.jetty.server.HttpChannel; import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.handler.AbstractHandler; @@ -75,34 +76,114 @@ public class ServerTimeoutsTest extends AbstractTest } @Test - public void testDelayedDispatchRequestWithDelayedFirstContentIdleTimeoutFires() throws Exception + public void testBlockingReadWithDelayedFirstContentWithUndelayedDispatchIdleTimeoutFires() throws Exception { - httpConfig.setDelayDispatchUntilContent(true); - CountDownLatch handlerLatch = new CountDownLatch(1); - start(new AbstractHandler.ErrorDispatchHandler() + testBlockingReadWithDelayedFirstContentIdleTimeoutFires(false); + } + + @Test + public void testBlockingReadWithDelayedFirstContentWithDelayedDispatchIdleTimeoutFires() throws Exception + { + testBlockingReadWithDelayedFirstContentIdleTimeoutFires(true); + } + + @Test + public void testAsyncReadWithDelayedFirstContentWithUndelayedDispatchIdleTimeoutFires() throws Exception + { + testAsyncReadWithDelayedFirstContentIdleTimeoutFires(false); + } + + @Test + public void testAsyncReadWithDelayedFirstContentWithDelayedDispatchIdleTimeoutFires() throws Exception + { + testAsyncReadWithDelayedFirstContentIdleTimeoutFires(true); + } + + private void testBlockingReadWithDelayedFirstContentIdleTimeoutFires(boolean delayDispatch) throws Exception + { + testReadWithDelayedFirstContentIdleTimeoutFires(new EmptyServerHandler() { @Override - protected void doNonErrorHandle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + protected void service(String target, Request jettyRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException { - baseRequest.setHandled(true); - handlerLatch.countDown(); + // The client did not send the content, + // idle timeout should result in IOException. + request.getInputStream().read(); } - }); - long idleTimeout = 2500; - setServerIdleTimeout(idleTimeout); + }, delayDispatch); + } - CountDownLatch resultLatch = new CountDownLatch(1); - client.POST(newURI()) - .content(new DeferredContentProvider()) - .send(result -> + private void testAsyncReadWithDelayedFirstContentIdleTimeoutFires(boolean delayDispatch) throws Exception + { + testReadWithDelayedFirstContentIdleTimeoutFires(new EmptyServerHandler() + { + @Override + protected void service(String target, Request jettyRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + { + AsyncContext asyncContext = request.startAsync(); + asyncContext.setTimeout(0); + request.getInputStream().setReadListener(new ReadListener() { - if (result.isFailed()) - resultLatch.countDown(); + @Override + public void onDataAvailable() + { + } + + @Override + public void onAllDataRead() + { + } + + @Override + public void onError(Throwable t) + { + if (t instanceof TimeoutException) + response.setStatus(HttpStatus.INTERNAL_SERVER_ERROR_500); + asyncContext.complete(); + } }); - // We did not send the content, the request was not - // dispatched, the server should have idle timed out. - Assert.assertFalse(handlerLatch.await(2 * idleTimeout, TimeUnit.MILLISECONDS)); + } + }, delayDispatch); + } + + private void testReadWithDelayedFirstContentIdleTimeoutFires(Handler handler, boolean delayDispatch) throws Exception + { + httpConfig.setDelayDispatchUntilContent(delayDispatch); + CountDownLatch handlerLatch = new CountDownLatch(1); + start(new AbstractHandler() + { + @Override + public void handle(String target, Request jettyRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + { + try + { + handler.handle(target, jettyRequest, request, response); + } + finally + { + handlerLatch.countDown(); + } + } + }); + long idleTimeout = 1000; + setServerIdleTimeout(idleTimeout); + + CountDownLatch resultLatch = new CountDownLatch(2); + DeferredContentProvider content = new DeferredContentProvider(); + client.POST(newURI()) + .content(content) + .onResponseSuccess(response -> + { + if (response.getStatus() == HttpStatus.INTERNAL_SERVER_ERROR_500) + resultLatch.countDown(); + content.close(); + }) + .send(result -> resultLatch.countDown()); + + // The client did not send the content, the request was + // dispatched, the server should have idle timed it out. + Assert.assertTrue(handlerLatch.await(2 * idleTimeout, TimeUnit.MILLISECONDS)); Assert.assertTrue(resultLatch.await(5, TimeUnit.SECONDS)); }