From 1693dd135d70b892da51a6a810e1a7379a64067e Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Tue, 8 Dec 2015 22:10:27 +0100 Subject: [PATCH] 483857 - jetty-client onComplete isn't called in case of exception in GZIPContentDecoder. Fixed by catching the exceptions and failing the callbacks. Also using return values from HttpReceiver to compute what to return to the parser. --- .../eclipse/jetty/client/HttpReceiver.java | 41 +++++++++++-------- .../client/http/HttpReceiverOverHTTP.java | 22 +++++----- .../jetty/client/HttpClientGZIPTest.java | 35 ++++++++++++++++ .../client/http/HttpConnectionOverFCGI.java | 7 ++-- .../jetty/util/CompletableCallback.java | 8 ++++ 5 files changed, 82 insertions(+), 31 deletions(-) diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpReceiver.java b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpReceiver.java index deb6069e52e..6c63293275d 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpReceiver.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpReceiver.java @@ -323,27 +323,34 @@ public abstract class HttpReceiver } else { - List decodeds = new ArrayList<>(2); - while (buffer.hasRemaining()) + try { - ByteBuffer decoded = decoder.decode(buffer); - if (!decoded.hasRemaining()) - continue; - decodeds.add(decoded); - if (LOG.isDebugEnabled()) - LOG.debug("Response content decoded ({}) {}{}{}", decoder, response, System.lineSeparator(), BufferUtil.toDetailString(decoded)); - } + List decodeds = new ArrayList<>(2); + while (buffer.hasRemaining()) + { + ByteBuffer decoded = decoder.decode(buffer); + if (!decoded.hasRemaining()) + continue; + decodeds.add(decoded); + if (LOG.isDebugEnabled()) + LOG.debug("Response content decoded ({}) {}{}{}", decoder, response, System.lineSeparator(), BufferUtil.toDetailString(decoded)); + } - if (decodeds.isEmpty()) - { - callback.succeeded(); + if (decodeds.isEmpty()) + { + callback.succeeded(); + } + else + { + int size = decodeds.size(); + CountingCallback counter = new CountingCallback(callback, size); + for (int i = 0; i < size; ++i) + notifier.notifyContent(listeners, response, decodeds.get(i), counter); + } } - else + catch (Throwable x) { - int size = decodeds.size(); - CountingCallback counter = new CountingCallback(callback, size); - for (int i = 0; i < size; ++i) - notifier.notifyContent(listeners, response, decodeds.get(i), counter); + callback.failed(x); } } diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpReceiverOverHTTP.java b/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpReceiverOverHTTP.java index d4d037844b3..02b6c37f0f5 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpReceiverOverHTTP.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpReceiverOverHTTP.java @@ -205,8 +205,7 @@ public class HttpReceiverOverHTTP extends HttpReceiver implements HttpParser.Res parser.setHeadResponse(HttpMethod.HEAD.is(method) || HttpMethod.CONNECT.is(method)); exchange.getResponse().version(version).status(status).reason(reason); - responseBegin(exchange); - return false; + return !responseBegin(exchange); } @Override @@ -216,8 +215,7 @@ public class HttpReceiverOverHTTP extends HttpReceiver implements HttpParser.Res if (exchange == null) return false; - responseHeader(exchange, field); - return false; + return !responseHeader(exchange, field); } @Override @@ -227,8 +225,7 @@ public class HttpReceiverOverHTTP extends HttpReceiver implements HttpParser.Res if (exchange == null) return false; - responseHeaders(exchange); - return false; + return !responseHeaders(exchange); } @Override @@ -253,17 +250,20 @@ public class HttpReceiverOverHTTP extends HttpReceiver implements HttpParser.Res failAndClose(x); } }; - responseContent(exchange, buffer, callback); - return callback.tryComplete(); + // Do not short circuit these calls. + boolean proceed = responseContent(exchange, buffer, callback); + boolean async = callback.tryComplete(); + return !proceed || async; } @Override public boolean messageComplete() { HttpExchange exchange = getHttpExchange(); - if (exchange != null) - responseSuccess(exchange); - return false; + if (exchange == null) + return false; + + return !responseSuccess(exchange); } @Override diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientGZIPTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientGZIPTest.java index bd45bb82018..d348c6cea3b 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientGZIPTest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientGZIPTest.java @@ -22,14 +22,18 @@ import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InterruptedIOException; import java.util.Arrays; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import java.util.zip.GZIPOutputStream; + import javax.servlet.ServletException; import javax.servlet.ServletOutputStream; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import org.eclipse.jetty.client.api.ContentResponse; +import org.eclipse.jetty.client.api.Response; +import org.eclipse.jetty.client.api.Result; import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.handler.AbstractHandler; import org.eclipse.jetty.util.ssl.SslContextFactory; @@ -195,6 +199,37 @@ public class HttpClientGZIPTest extends AbstractHttpClientServerTest Assert.assertArrayEquals(data, response.getContent()); } + @Test + public void testGZIPContentCorrupted() throws Exception + { + start(new AbstractHandler() + { + @Override + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + { + baseRequest.setHandled(true); + response.setHeader("Content-Encoding", "gzip"); + // Not gzipped, will cause the client to blow up. + response.getOutputStream().print("0123456789"); + } + }); + + final CountDownLatch latch = new CountDownLatch(1); + client.newRequest("localhost", connector.getLocalPort()) + .scheme(scheme) + .send(new Response.CompleteListener() + { + @Override + public void onComplete(Result result) + { + if (result.isFailed()) + latch.countDown(); + } + }); + + Assert.assertTrue(latch.await(5, TimeUnit.SECONDS)); + } + private static void sleep(long ms) throws IOException { try diff --git a/jetty-fcgi/fcgi-client/src/main/java/org/eclipse/jetty/fcgi/client/http/HttpConnectionOverFCGI.java b/jetty-fcgi/fcgi-client/src/main/java/org/eclipse/jetty/fcgi/client/http/HttpConnectionOverFCGI.java index 8d7c224da96..e68999c4c36 100644 --- a/jetty-fcgi/fcgi-client/src/main/java/org/eclipse/jetty/fcgi/client/http/HttpConnectionOverFCGI.java +++ b/jetty-fcgi/fcgi-client/src/main/java/org/eclipse/jetty/fcgi/client/http/HttpConnectionOverFCGI.java @@ -377,9 +377,10 @@ public class HttpConnectionOverFCGI extends AbstractConnection implements Connec close(x); } }; - if (!channel.content(buffer, callback)) - return true; - return callback.tryComplete(); + // Do not short circuit these calls. + boolean proceed = channel.content(buffer, callback); + boolean async = callback.tryComplete(); + return !proceed || async; } else { diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/CompletableCallback.java b/jetty-util/src/main/java/org/eclipse/jetty/util/CompletableCallback.java index 1020bb6267e..5c401341dc2 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/CompletableCallback.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/CompletableCallback.java @@ -82,6 +82,10 @@ public abstract class CompletableCallback implements Callback } break; } + case FAILED: + { + return; + } default: { throw new IllegalStateException(current.toString()); @@ -108,6 +112,10 @@ public abstract class CompletableCallback implements Callback } break; } + case FAILED: + { + return; + } default: { throw new IllegalStateException(current.toString());