From cae4204150572d7e5c38085b682b28af707305b8 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Mon, 21 Jul 2014 16:46:32 +0200 Subject: [PATCH] 440020 - ProxyServlet does not handle correctly failure after committed response to client. Fixed by introducing a request attribute "org.eclipse.jetty.server .Response.failure" used by HttpChannel to immediately close the connection when it sees it. --- .../org/eclipse/jetty/proxy/ProxyServlet.java | 10 +- .../eclipse/jetty/proxy/ProxyServletTest.java | 157 +++++++++++++++++- .../org/eclipse/jetty/server/HttpChannel.java | 21 ++- .../eclipse/jetty/server/HttpConnection.java | 2 +- 4 files changed, 185 insertions(+), 5 deletions(-) diff --git a/jetty-proxy/src/main/java/org/eclipse/jetty/proxy/ProxyServlet.java b/jetty-proxy/src/main/java/org/eclipse/jetty/proxy/ProxyServlet.java index e18e95dfa10..3e93592d8db 100644 --- a/jetty-proxy/src/main/java/org/eclipse/jetty/proxy/ProxyServlet.java +++ b/jetty-proxy/src/main/java/org/eclipse/jetty/proxy/ProxyServlet.java @@ -34,6 +34,7 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import javax.servlet.AsyncContext; import javax.servlet.ServletConfig; +import javax.servlet.ServletContext; import javax.servlet.ServletException; import javax.servlet.UnavailableException; import javax.servlet.http.HttpServlet; @@ -559,8 +560,15 @@ public class ProxyServlet extends HttpServlet protected void onResponseFailure(HttpServletRequest request, HttpServletResponse response, Response proxyResponse, Throwable failure) { _log.debug(getRequestId(request) + " proxying failed", failure); - if (!response.isCommitted()) + if (response.isCommitted()) { + request.setAttribute("org.eclipse.jetty.server.Response.failure", failure); + AsyncContext asyncContext = request.getAsyncContext(); + asyncContext.complete(); + } + else + { + response.resetBuffer(); if (failure instanceof TimeoutException) response.setStatus(HttpServletResponse.SC_GATEWAY_TIMEOUT); else diff --git a/jetty-proxy/src/test/java/org/eclipse/jetty/proxy/ProxyServletTest.java b/jetty-proxy/src/test/java/org/eclipse/jetty/proxy/ProxyServletTest.java index 5884a14f829..1eec08a483f 100644 --- a/jetty-proxy/src/test/java/org/eclipse/jetty/proxy/ProxyServletTest.java +++ b/jetty-proxy/src/test/java/org/eclipse/jetty/proxy/ProxyServletTest.java @@ -19,8 +19,10 @@ package org.eclipse.jetty.proxy; import java.io.ByteArrayOutputStream; +import java.io.EOFException; import java.io.IOException; import java.io.InputStream; +import java.io.InterruptedIOException; import java.io.OutputStream; import java.io.PrintWriter; import java.net.ConnectException; @@ -43,6 +45,7 @@ import javax.servlet.AsyncContext; import javax.servlet.AsyncEvent; import javax.servlet.AsyncListener; import javax.servlet.ServletException; +import javax.servlet.ServletOutputStream; import javax.servlet.http.Cookie; import javax.servlet.http.HttpServlet; import javax.servlet.http.HttpServletRequest; @@ -55,9 +58,13 @@ import org.eclipse.jetty.client.api.ContentResponse; import org.eclipse.jetty.client.api.Request; import org.eclipse.jetty.client.api.Response; import org.eclipse.jetty.client.api.Result; +import org.eclipse.jetty.client.http.HttpDestinationOverHTTP; import org.eclipse.jetty.client.util.BufferingResponseListener; import org.eclipse.jetty.client.util.BytesContentProvider; +import org.eclipse.jetty.client.util.InputStreamResponseListener; import org.eclipse.jetty.http.HttpMethod; +import org.eclipse.jetty.server.HttpConfiguration; +import org.eclipse.jetty.server.HttpConnectionFactory; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.ServerConnector; import org.eclipse.jetty.servlet.ServletContextHandler; @@ -112,7 +119,12 @@ public class ProxyServletTest private void prepareProxy(Map initParams) throws Exception { proxy = new Server(); - proxyConnector = new ServerConnector(proxy); + + HttpConfiguration configuration = new HttpConfiguration(); + String value = initParams.get("outputBufferSize"); + if (value != null) + configuration.setOutputBufferSize(Integer.valueOf(value)); + proxyConnector = new ServerConnector(proxy, new HttpConnectionFactory(configuration)); proxy.addConnector(proxyConnector); ServletContextHandler proxyCtx = new ServletContextHandler(proxy, "/", true, false); @@ -899,5 +911,148 @@ public class ProxyServletTest Assert.assertTrue(response3.getHeaders().containsKey(PROXIED_HEADER)); } + @Test + public void testProxyRequestFailureInTheMiddleOfProxyingSmallContent() throws Exception + { + final long proxyTimeout = 1000; + Map proxyParams = new HashMap<>(); + proxyParams.put("timeout", String.valueOf(proxyTimeout)); + prepareProxy(proxyParams); + + final CountDownLatch chunk1Latch = new CountDownLatch(1); + final int chunk1 = 'q'; + final int chunk2 = 'w'; + prepareServer(new HttpServlet() + { + @Override + protected void service(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException + { + ServletOutputStream output = response.getOutputStream(); + output.write(chunk1); + response.flushBuffer(); + + // Wait for the client to receive this chunk. + await(chunk1Latch, 5000); + + // Send second chunk, must not be received by proxy. + output.write(chunk2); + } + + private boolean await(CountDownLatch latch, long ms) throws IOException + { + try + { + return latch.await(ms, TimeUnit.MILLISECONDS); + } + catch (InterruptedException x) + { + throw new InterruptedIOException(); + } + } + }); + + HttpClient client = prepareClient(); + InputStreamResponseListener listener = new InputStreamResponseListener(); + int port = serverConnector.getLocalPort(); + client.newRequest("localhost", port).send(listener); + + // Make the proxy request fail; given the small content, the + // proxy-to-client response is not committed yet so it will be reset. + TimeUnit.MILLISECONDS.sleep(2 * proxyTimeout); + + Response response = listener.get(5, TimeUnit.SECONDS); + Assert.assertEquals(504, response.getStatus()); + + // Make sure there is no content, as the proxy-to-client response has been reset. + InputStream input = listener.getInputStream(); + Assert.assertEquals(-1, input.read()); + + chunk1Latch.countDown(); + + // Result succeeds because a 504 is a valid HTTP response. + Result result = listener.await(5, TimeUnit.SECONDS); + Assert.assertTrue(result.isSucceeded()); + + // Make sure the proxy does not receive chunk2. + Assert.assertEquals(-1, input.read()); + + HttpDestinationOverHTTP destination = (HttpDestinationOverHTTP)client.getDestination("http", "localhost", port); + Assert.assertEquals(0, destination.getConnectionPool().getIdleConnections().size()); + } + + @Test + public void testProxyRequestFailureInTheMiddleOfProxyingBigContent() throws Exception + { + final long proxyTimeout = 1000; + int outputBufferSize = 1024; + Map proxyParams = new HashMap<>(); + proxyParams.put("timeout", String.valueOf(proxyTimeout)); + proxyParams.put("outputBufferSize", String.valueOf(outputBufferSize)); + prepareProxy(proxyParams); + + final CountDownLatch chunk1Latch = new CountDownLatch(1); + final byte[] chunk1 = new byte[outputBufferSize]; + new Random().nextBytes(chunk1); + final int chunk2 = 'w'; + prepareServer(new HttpServlet() + { + @Override + protected void service(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException + { + ServletOutputStream output = response.getOutputStream(); + output.write(chunk1); + response.flushBuffer(); + + // Wait for the client to receive this chunk. + await(chunk1Latch, 5000); + + // Send second chunk, must not be received by proxy. + output.write(chunk2); + } + + private boolean await(CountDownLatch latch, long ms) throws IOException + { + try + { + return latch.await(ms, TimeUnit.MILLISECONDS); + } + catch (InterruptedException x) + { + throw new InterruptedIOException(); + } + } + }); + + HttpClient client = prepareClient(); + InputStreamResponseListener listener = new InputStreamResponseListener(); + int port = serverConnector.getLocalPort(); + client.newRequest("localhost", port).send(listener); + + Response response = listener.get(5, TimeUnit.SECONDS); + Assert.assertEquals(200, response.getStatus()); + + InputStream input = listener.getInputStream(); + for (int i = 0; i < chunk1.length; ++i) + Assert.assertEquals(chunk1[i] & 0xFF, input.read()); + + TimeUnit.MILLISECONDS.sleep(2 * proxyTimeout); + + chunk1Latch.countDown(); + + try + { + // Make sure the proxy does not receive chunk2. + input.read(); + Assert.fail(); + } + catch (EOFException x) + { + // Expected + } + + HttpDestinationOverHTTP destination = (HttpDestinationOverHTTP)client.getDestination("http", "localhost", port); + Assert.assertEquals(0, destination.getConnectionPool().getIdleConnections().size()); + } + // TODO: test proxy authentication } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java index e86001ed542..e6abb983113 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java @@ -397,10 +397,27 @@ public class HttpChannel implements HttpParser.RequestHandler, Runnable, H _state.completed(); if (!_response.isCommitted() && !_request.isHandled()) + { _response.sendError(404); + } else - // Complete generating the response - _response.closeOutput(); + { + // There is no way in the Servlet API to directly close a connection, + // so we rely on applications to pass this attribute to signal they + // want to hard close the connection, without even closing the output. + Object failure = _request.getAttribute("org.eclipse.jetty.server.Response.failure"); + if (failure != null) + { + if (LOG.isDebugEnabled()) + LOG.debug("Explicit response failure", failure); + failed(); + } + else + { + // Complete generating the response + _response.closeOutput(); + } + } } catch(EofException|ClosedChannelException e) { 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 3ccb663e40c..ca8ea64e9be 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 @@ -539,9 +539,9 @@ public class HttpConnection extends AbstractConnection implements Runnable, Http @Override public void failed() { + _generator.setPersistent(false); getEndPoint().shutdownOutput(); } - @Override public boolean messageComplete()