From 8f53d14e15aa27bc48024bfba1bbaada3630c20b Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Wed, 12 Jun 2019 10:51:15 +0200 Subject: [PATCH 1/6] Issue #3758 - Avoid sending empty trailer frames for http/2 requests. Modified the sender logic to allow specific subclasses to decide when to send the trailers, if any. This allows HTTP/2 to correctly compute the end_stream flag and avoid sending empty trailers frames with end_stream=true. Signed-off-by: Simone Bordet --- .../org/eclipse/jetty/client/HttpSender.java | 54 +------ .../jetty/client/http/HttpSenderOverHTTP.java | 122 ++------------- .../fcgi/client/http/HttpSenderOverFCGI.java | 6 - .../jetty/http2/client/TrailersTest.java | 22 +-- .../client/http/HttpSenderOverHTTP2.java | 129 +++++++++++----- .../client/http/RequestTrailersTest.java | 142 ++++++++++++++++++ 6 files changed, 259 insertions(+), 216 deletions(-) create mode 100644 jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/RequestTrailersTest.java diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpSender.java b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpSender.java index 694ff157509..99b865bd70a 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpSender.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpSender.java @@ -22,12 +22,10 @@ import java.nio.ByteBuffer; import java.util.concurrent.Executor; import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.atomic.AtomicReference; -import java.util.function.Supplier; import org.eclipse.jetty.client.api.ContentProvider; import org.eclipse.jetty.client.api.Request; import org.eclipse.jetty.client.api.Result; -import org.eclipse.jetty.http.HttpFields; import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.HttpHeaderValue; import org.eclipse.jetty.util.BufferUtil; @@ -67,7 +65,6 @@ public abstract class HttpSender implements AsyncContentProvider.Listener private final AtomicReference senderState = new AtomicReference<>(SenderState.IDLE); private final Callback commitCallback = new CommitCallback(); private final IteratingCallback contentCallback = new ContentCallback(); - private final Callback trailersCallback = new TrailersCallback(); private final Callback lastCallback = new LastCallback(); private final HttpChannel channel; private HttpContent content; @@ -444,15 +441,6 @@ public abstract class HttpSender implements AsyncContentProvider.Listener */ protected abstract void sendContent(HttpExchange exchange, HttpContent content, Callback callback); - /** - * Implementations should send the HTTP trailers and notify the given {@code callback} of the - * result of this operation. - * - * @param exchange the exchange to send - * @param callback the callback to notify - */ - protected abstract void sendTrailers(HttpExchange exchange, Callback callback); - protected void reset() { HttpContent content = this.content; @@ -745,20 +733,10 @@ public abstract class HttpSender implements AsyncContentProvider.Listener if (content == null) return; - HttpRequest request = exchange.getRequest(); - Supplier trailers = request.getTrailers(); - boolean hasContent = content.hasContent(); - if (!hasContent) + if (!content.hasContent()) { - if (trailers == null) - { - // No trailers or content to send, we are done. - someToSuccess(exchange); - } - else - { - sendTrailers(exchange, lastCallback); - } + // No content to send, we are done. + someToSuccess(exchange); } else { @@ -859,9 +837,7 @@ public abstract class HttpSender implements AsyncContentProvider.Listener if (lastContent) { - HttpRequest request = exchange.getRequest(); - Supplier trailers = request.getTrailers(); - sendContent(exchange, content, trailers == null ? lastCallback : trailersCallback); + sendContent(exchange, content, lastCallback); return Action.IDLE; } @@ -925,28 +901,6 @@ public abstract class HttpSender implements AsyncContentProvider.Listener } } - private class TrailersCallback implements Callback - { - @Override - public void succeeded() - { - HttpExchange exchange = getHttpExchange(); - if (exchange == null) - return; - sendTrailers(exchange, lastCallback); - } - - @Override - public void failed(Throwable x) - { - HttpContent content = HttpSender.this.content; - if (content == null) - return; - content.failed(x); - anyToFailure(x); - } - } - private class LastCallback implements Callback { @Override diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpSenderOverHTTP.java b/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpSenderOverHTTP.java index 4cab4008904..1f8d04397da 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpSenderOverHTTP.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpSenderOverHTTP.java @@ -59,7 +59,7 @@ public class HttpSenderOverHTTP extends HttpSender { try { - new HeadersCallback(exchange, content, callback, getHttpChannel().getHttpConnection()).iterate(); + new HeadersCallback(exchange, content, callback).iterate(); } catch (Throwable x) { @@ -83,8 +83,8 @@ public class HttpSenderOverHTTP extends HttpSender HttpGenerator.Result result = generator.generateRequest(null, null, chunk, contentBuffer, lastContent); if (LOG.isDebugEnabled()) LOG.debug("Generated content ({} bytes) - {}/{}", - contentBuffer == null ? -1 : contentBuffer.remaining(), - result, generator); + contentBuffer == null ? -1 : contentBuffer.remaining(), + result, generator); switch (result) { case NEED_CHUNK: @@ -138,21 +138,6 @@ public class HttpSenderOverHTTP extends HttpSender } } - @Override - protected void sendTrailers(HttpExchange exchange, Callback callback) - { - try - { - new TrailersCallback(callback).iterate(); - } - catch (Throwable x) - { - if (LOG.isDebugEnabled()) - LOG.debug(x); - callback.failed(x); - } - } - @Override protected void reset() { @@ -191,19 +176,17 @@ public class HttpSenderOverHTTP extends HttpSender private final HttpExchange exchange; private final Callback callback; private final MetaData.Request metaData; - private final HttpConnectionOverHTTP httpConnectionOverHTTP; private ByteBuffer headerBuffer; private ByteBuffer chunkBuffer; private ByteBuffer contentBuffer; private boolean lastContent; private boolean generated; - public HeadersCallback(HttpExchange exchange, HttpContent content, Callback callback, HttpConnectionOverHTTP httpConnectionOverHTTP) + public HeadersCallback(HttpExchange exchange, HttpContent content, Callback callback) { super(false); this.exchange = exchange; this.callback = callback; - this.httpConnectionOverHTTP = httpConnectionOverHTTP; HttpRequest request = exchange.getRequest(); ContentProvider requestContent = request.getContent(); @@ -231,10 +214,10 @@ public class HttpSenderOverHTTP extends HttpSender HttpGenerator.Result result = generator.generateRequest(metaData, headerBuffer, chunkBuffer, contentBuffer, lastContent); if (LOG.isDebugEnabled()) LOG.debug("Generated headers ({} bytes), chunk ({} bytes), content ({} bytes) - {}/{}", - headerBuffer == null ? -1 : headerBuffer.remaining(), - chunkBuffer == null ? -1 : chunkBuffer.remaining(), - contentBuffer == null ? -1 : contentBuffer.remaining(), - result, generator); + headerBuffer == null ? -1 : headerBuffer.remaining(), + chunkBuffer == null ? -1 : chunkBuffer.remaining(), + contentBuffer == null ? -1 : contentBuffer.remaining(), + result, generator); switch (result) { case NEED_HEADER: @@ -249,7 +232,8 @@ public class HttpSenderOverHTTP extends HttpSender } case NEED_CHUNK_TRAILER: { - return Action.SUCCEEDED; + chunkBuffer = httpClient.getByteBufferPool().acquire(httpClient.getRequestBufferSize(), false); + break; } case FLUSH: { @@ -260,11 +244,8 @@ public class HttpSenderOverHTTP extends HttpSender chunkBuffer = BufferUtil.EMPTY_BUFFER; if (contentBuffer == null) contentBuffer = BufferUtil.EMPTY_BUFFER; - - httpConnectionOverHTTP.addBytesOut( BufferUtil.length(headerBuffer) - + BufferUtil.length(chunkBuffer) - + BufferUtil.length(contentBuffer)); - + long bytes = headerBuffer.remaining() + chunkBuffer.remaining() + contentBuffer.remaining(); + getHttpChannel().getHttpConnection().addBytesOut(bytes); endPoint.write(this, headerBuffer, chunkBuffer, contentBuffer); generated = true; return Action.SCHEDULED; @@ -331,83 +312,6 @@ public class HttpSenderOverHTTP extends HttpSender } } - private class TrailersCallback extends IteratingCallback - { - private final Callback callback; - private ByteBuffer chunkBuffer; - - public TrailersCallback(Callback callback) - { - this.callback = callback; - } - - @Override - protected Action process() throws Throwable - { - while (true) - { - HttpGenerator.Result result = generator.generateRequest(null, null, chunkBuffer, null, true); - if (LOG.isDebugEnabled()) - LOG.debug("Generated trailers {}/{}", result, generator); - switch (result) - { - case NEED_CHUNK_TRAILER: - { - chunkBuffer = httpClient.getByteBufferPool().acquire(httpClient.getRequestBufferSize(), false); - break; - } - case FLUSH: - { - EndPoint endPoint = getHttpChannel().getHttpConnection().getEndPoint(); - endPoint.write(this, chunkBuffer); - return Action.SCHEDULED; - } - case SHUTDOWN_OUT: - { - shutdownOutput(); - return Action.SUCCEEDED; - } - case DONE: - { - return Action.SUCCEEDED; - } - default: - { - throw new IllegalStateException(result.toString()); - } - } - } - } - - @Override - public void succeeded() - { - release(); - super.succeeded(); - } - - @Override - public void failed(Throwable x) - { - release(); - callback.failed(x); - super.failed(x); - } - - @Override - protected void onCompleteSuccess() - { - super.onCompleteSuccess(); - callback.succeeded(); - } - - private void release() - { - httpClient.getByteBufferPool().release(chunkBuffer); - chunkBuffer = null; - } - } - private class ByteBufferRecyclerCallback extends Callback.Nested { private final ByteBufferPool pool; @@ -435,7 +339,9 @@ public class HttpSenderOverHTTP extends HttpSender public void failed(Throwable x) { for (ByteBuffer buffer : buffers) + { pool.release(buffer); + } super.failed(x); } } diff --git a/jetty-fcgi/fcgi-client/src/main/java/org/eclipse/jetty/fcgi/client/http/HttpSenderOverFCGI.java b/jetty-fcgi/fcgi-client/src/main/java/org/eclipse/jetty/fcgi/client/http/HttpSenderOverFCGI.java index 3a5a6b63a4d..93203f26d00 100644 --- a/jetty-fcgi/fcgi-client/src/main/java/org/eclipse/jetty/fcgi/client/http/HttpSenderOverFCGI.java +++ b/jetty-fcgi/fcgi-client/src/main/java/org/eclipse/jetty/fcgi/client/http/HttpSenderOverFCGI.java @@ -125,10 +125,4 @@ public class HttpSenderOverFCGI extends HttpSender getHttpChannel().flush(result); } } - - @Override - protected void sendTrailers(HttpExchange exchange, Callback callback) - { - callback.succeeded(); - } } diff --git a/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/TrailersTest.java b/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/TrailersTest.java index 5b81e21bf20..348bb41d02b 100644 --- a/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/TrailersTest.java +++ b/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/TrailersTest.java @@ -18,16 +18,6 @@ package org.eclipse.jetty.http2.client; -import static org.hamcrest.Matchers.is; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertNull; -import static org.junit.jupiter.api.Assertions.assertTrue; - -import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.equalTo; - import java.io.IOException; import java.nio.ByteBuffer; import java.nio.charset.StandardCharsets; @@ -35,7 +25,6 @@ import java.util.ArrayList; import java.util.List; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; - import javax.servlet.ServletInputStream; import javax.servlet.http.HttpServlet; import javax.servlet.http.HttpServletRequest; @@ -58,10 +47,14 @@ import org.eclipse.jetty.server.Response; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.FuturePromise; import org.eclipse.jetty.util.Promise; - import org.eclipse.jetty.util.StringUtil; import org.junit.jupiter.api.Test; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; public class TrailersTest extends AbstractTest { @@ -289,7 +282,7 @@ public class TrailersTest extends AbstractTest assertTrue(latch.await(5, TimeUnit.SECONDS)); - assertTrue( frames.size()==3, frames.toString()); + assertEquals(3, frames.size(), frames.toString()); HeadersFrame headers = (HeadersFrame)frames.get(0); DataFrame data = (DataFrame)frames.get(1); @@ -298,7 +291,7 @@ public class TrailersTest extends AbstractTest assertFalse(headers.isEndStream()); assertFalse(data.isEndStream()); assertTrue(trailers.isEndStream()); - assertTrue(trailers.getMetaData().getFields().get(trailerName).equals(trailerValue)); + assertEquals(trailers.getMetaData().getFields().get(trailerName), trailerValue); } @Test @@ -358,6 +351,5 @@ public class TrailersTest extends AbstractTest assertTrue(serverLatch.await(5, TimeUnit.SECONDS)); assertTrue(clientLatch.await(5, TimeUnit.SECONDS)); - } } diff --git a/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpSenderOverHTTP2.java b/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpSenderOverHTTP2.java index b37d77a8b94..e376a91eeac 100644 --- a/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpSenderOverHTTP2.java +++ b/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpSenderOverHTTP2.java @@ -56,42 +56,57 @@ public class HttpSenderOverHTTP2 extends HttpSender String path = relativize(request.getPath()); HttpURI uri = HttpURI.createHttpURI(request.getScheme(), request.getHost(), request.getPort(), path, null, request.getQuery(), null); MetaData.Request metaData = new MetaData.Request(request.getMethod(), uri, HttpVersion.HTTP_2, request.getHeaders()); - Supplier trailers = request.getTrailers(); - metaData.setTrailerSupplier(trailers); - HeadersFrame headersFrame = new HeadersFrame(metaData, null, trailers == null && !content.hasContent()); - HttpChannelOverHTTP2 channel = getHttpChannel(); - Promise promise = new Promise() - { - @Override - public void succeeded(Stream stream) - { - channel.setStream(stream); - ((IStream)stream).setAttachment(channel); - long idleTimeout = request.getIdleTimeout(); - if (idleTimeout >= 0) - stream.setIdleTimeout(idleTimeout); + Supplier trailerSupplier = request.getTrailers(); + metaData.setTrailerSupplier(trailerSupplier); - if (content.hasContent() && !expects100Continue(request)) + HeadersFrame headersFrame; + Promise promise; + if (content.hasContent()) + { + headersFrame = new HeadersFrame(metaData, null, false); + promise = new HeadersPromise(request, callback) + { + @Override + public void succeeded(Stream stream) { - boolean advanced = content.advance(); - boolean lastContent = trailers == null && content.isLast(); - if (advanced || lastContent) + super.succeeded(stream); + if (expects100Continue(request)) { - DataFrame dataFrame = new DataFrame(stream.getId(), content.getByteBuffer(), lastContent); - stream.data(dataFrame, callback); - return; + // Don't send the content yet. + callback.succeeded(); + } + else + { + boolean advanced = content.advance(); + boolean lastContent = content.isLast(); + if (advanced || lastContent) + sendContent(stream, content, trailerSupplier, callback); + else + callback.succeeded(); } } - callback.succeeded(); - } - - @Override - public void failed(Throwable failure) + }; + } + else + { + HttpFields trailers = trailerSupplier == null ? null : trailerSupplier.get(); + boolean endStream = trailers == null || trailers.size() == 0; + headersFrame = new HeadersFrame(metaData, null, endStream); + promise = new HeadersPromise(request, callback) { - callback.failed(failure); - } - }; + @Override + public void succeeded(Stream stream) + { + super.succeeded(stream); + if (endStream) + callback.succeeded(); + else + sendTrailers(stream, trailers, callback); + } + }; + } // TODO optimize the send of HEADERS and DATA frames. + HttpChannelOverHTTP2 channel = getHttpChannel(); channel.getSession().newStream(headersFrame, promise, channel.getStreamListener()); } @@ -123,19 +138,59 @@ public class HttpSenderOverHTTP2 extends HttpSender else { Stream stream = getHttpChannel().getStream(); - Supplier trailers = exchange.getRequest().getTrailers(); - DataFrame frame = new DataFrame(stream.getId(), content.getByteBuffer(), trailers == null && content.isLast()); - stream.data(frame, callback); + Supplier trailerSupplier = exchange.getRequest().getTrailers(); + sendContent(stream, content, trailerSupplier, callback); } } - @Override - protected void sendTrailers(HttpExchange exchange, Callback callback) + private void sendContent(Stream stream, HttpContent content, Supplier trailerSupplier, Callback callback) { - Supplier trailers = exchange.getRequest().getTrailers(); - MetaData metaData = new MetaData(HttpVersion.HTTP_2, trailers.get()); - Stream stream = getHttpChannel().getStream(); + boolean lastContent = content.isLast(); + HttpFields trailers = null; + boolean endStream = false; + if (lastContent) + { + trailers = trailerSupplier == null ? null : trailerSupplier.get(); + endStream = trailers == null || trailers.size() == 0; + } + DataFrame dataFrame = new DataFrame(stream.getId(), content.getByteBuffer(), endStream); + HttpFields fTrailers = trailers; + stream.data(dataFrame, endStream || !lastContent ? callback : Callback.from(() -> sendTrailers(stream, fTrailers, callback), callback::failed)); + } + + private void sendTrailers(Stream stream, HttpFields trailers, Callback callback) + { + MetaData metaData = new MetaData(HttpVersion.HTTP_2, trailers); HeadersFrame trailersFrame = new HeadersFrame(stream.getId(), metaData, null, true); stream.headers(trailersFrame, callback); } + + private class HeadersPromise implements Promise + { + private final HttpRequest request; + private final Callback callback; + + private HeadersPromise(HttpRequest request, Callback callback) + { + this.request = request; + this.callback = callback; + } + + @Override + public void succeeded(Stream stream) + { + HttpChannelOverHTTP2 channel = getHttpChannel(); + channel.setStream(stream); + ((IStream)stream).setAttachment(channel); + long idleTimeout = request.getIdleTimeout(); + if (idleTimeout >= 0) + stream.setIdleTimeout(idleTimeout); + } + + @Override + public void failed(Throwable x) + { + callback.failed(x); + } + } } diff --git a/jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/RequestTrailersTest.java b/jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/RequestTrailersTest.java new file mode 100644 index 00000000000..e6d321632d3 --- /dev/null +++ b/jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/RequestTrailersTest.java @@ -0,0 +1,142 @@ +// +// ======================================================================== +// Copyright (c) 1995-2019 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.http2.client.http; + +import java.nio.ByteBuffer; +import java.nio.charset.StandardCharsets; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; + +import org.eclipse.jetty.client.HttpRequest; +import org.eclipse.jetty.client.api.ContentResponse; +import org.eclipse.jetty.client.util.DeferredContentProvider; +import org.eclipse.jetty.client.util.StringContentProvider; +import org.eclipse.jetty.http.HttpFields; +import org.eclipse.jetty.http.HttpStatus; +import org.eclipse.jetty.http.HttpVersion; +import org.eclipse.jetty.http.MetaData; +import org.eclipse.jetty.http2.api.Stream; +import org.eclipse.jetty.http2.api.server.ServerSessionListener; +import org.eclipse.jetty.http2.frames.DataFrame; +import org.eclipse.jetty.http2.frames.HeadersFrame; +import org.eclipse.jetty.util.Callback; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class RequestTrailersTest extends AbstractTest +{ + @Test + public void testEmptyTrailersWithoutContent() throws Exception + { + testEmptyTrailers(null); + } + + @Test + public void testEmptyTrailersWithEagerContent() throws Exception + { + testEmptyTrailers("eager_content"); + } + + private void testEmptyTrailers(String content) throws Exception + { + CountDownLatch trailersLatch = new CountDownLatch(1); + start(new ServerSessionListener.Adapter() + { + @Override + public Stream.Listener onNewStream(Stream stream, HeadersFrame frame) + { + MetaData.Response response = new MetaData.Response(HttpVersion.HTTP_2, HttpStatus.OK_200, new HttpFields()); + HeadersFrame responseFrame = new HeadersFrame(stream.getId(), response, null, true); + stream.headers(responseFrame, Callback.NOOP); + return new Stream.Listener.Adapter() + { + @Override + public void onHeaders(Stream stream, HeadersFrame frame) + { + trailersLatch.countDown(); + } + }; + } + }); + + HttpRequest request = (HttpRequest)client.newRequest("localhost", connector.getLocalPort()); + HttpFields trailers = new HttpFields(); + request.trailers(() -> trailers); + if (content != null) + request.content(new StringContentProvider(content)); + + ContentResponse response = request.send(); + assertEquals(HttpStatus.OK_200, response.getStatus()); + + // The client must not send the trailers. + assertFalse(trailersLatch.await(1, TimeUnit.SECONDS)); + } + + @Test + public void testEmptyTrailersWithDeferredContent() throws Exception + { + start(new ServerSessionListener.Adapter() + { + @Override + public Stream.Listener onNewStream(Stream stream, HeadersFrame frame) + { + return new Stream.Listener.Adapter() + { + @Override + public void onData(Stream stream, DataFrame dataFrame, Callback callback) + { + callback.succeeded(); + // We should not receive an empty HEADERS frame for the + // trailers, but instead a DATA frame with endStream=true. + if (dataFrame.isEndStream()) + { + MetaData.Response response = new MetaData.Response(HttpVersion.HTTP_2, HttpStatus.OK_200, new HttpFields()); + HeadersFrame responseFrame = new HeadersFrame(stream.getId(), response, null, true); + stream.headers(responseFrame, Callback.NOOP); + } + } + }; + } + }); + + HttpRequest request = (HttpRequest)client.newRequest("localhost", connector.getLocalPort()); + HttpFields trailers = new HttpFields(); + request.trailers(() -> trailers); + DeferredContentProvider content = new DeferredContentProvider(); + request.content(content); + + CountDownLatch latch = new CountDownLatch(1); + request.send(result -> + { + assertTrue(result.isSucceeded()); + assertEquals(HttpStatus.OK_200, result.getResponse().getStatus()); + latch.countDown(); + }); + + // Send deferred content after a while. + Thread.sleep(1000); + content.offer(ByteBuffer.wrap("deferred_content".getBytes(StandardCharsets.UTF_8))); + content.close(); + + assertTrue(latch.await(5, TimeUnit.SECONDS)); + } +} From 82f7647629022e9152087012db672ea54008d6c8 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Wed, 12 Jun 2019 19:15:15 +0200 Subject: [PATCH 2/6] Issue #3758 - Avoid sending empty trailer frames for http/2 requests. Added one more test case and comments about handling of `content.isConsumed()` in HTTP/2. Signed-off-by: Simone Bordet --- .../org/eclipse/jetty/client/HttpSender.java | 2 +- .../jetty/client/http/HttpSenderOverHTTP.java | 4 +- .../client/http/HttpSenderOverHTTP2.java | 3 ++ .../client/http/RequestTrailersTest.java | 48 +++++++++++++++++++ 4 files changed, 54 insertions(+), 3 deletions(-) diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpSender.java b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpSender.java index 99b865bd70a..d29c09663b8 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpSender.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpSender.java @@ -740,7 +740,7 @@ public abstract class HttpSender implements AsyncContentProvider.Listener } else { - // Was any content sent while committing ? + // Was any content sent while committing? ByteBuffer contentBuffer = content.getContent(); if (contentBuffer != null) { diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpSenderOverHTTP.java b/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpSenderOverHTTP.java index 1f8d04397da..1edae133609 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpSenderOverHTTP.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpSenderOverHTTP.java @@ -94,8 +94,8 @@ public class HttpSenderOverHTTP extends HttpSender } case NEED_CHUNK_TRAILER: { - callback.succeeded(); - return; + chunk = bufferPool.acquire(httpClient.getRequestBufferSize(), false); + break; } case FLUSH: { diff --git a/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpSenderOverHTTP2.java b/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpSenderOverHTTP2.java index e376a91eeac..e1078ad0976 100644 --- a/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpSenderOverHTTP2.java +++ b/jetty-http2/http2-http-client-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpSenderOverHTTP2.java @@ -133,6 +133,9 @@ public class HttpSenderOverHTTP2 extends HttpSender { if (content.isConsumed()) { + // The superclass calls sendContent() one more time after the last content. + // This is necessary for HTTP/1.1 to generate the terminal chunk (with trailers), + // but it's not necessary for HTTP/2 so we just succeed the callback. callback.succeeded(); } else diff --git a/jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/RequestTrailersTest.java b/jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/RequestTrailersTest.java index e6d321632d3..46778f30d9a 100644 --- a/jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/RequestTrailersTest.java +++ b/jetty-http2/http2-http-client-transport/src/test/java/org/eclipse/jetty/http2/client/http/RequestTrailersTest.java @@ -139,4 +139,52 @@ public class RequestTrailersTest extends AbstractTest assertTrue(latch.await(5, TimeUnit.SECONDS)); } + + @Test + public void testEmptyTrailersWithEmptyDeferredContent() throws Exception + { + start(new ServerSessionListener.Adapter() + { + @Override + public Stream.Listener onNewStream(Stream stream, HeadersFrame frame) + { + return new Stream.Listener.Adapter() + { + @Override + public void onData(Stream stream, DataFrame dataFrame, Callback callback) + { + callback.succeeded(); + // We should not receive an empty HEADERS frame for the + // trailers, but instead a DATA frame with endStream=true. + if (dataFrame.isEndStream()) + { + MetaData.Response response = new MetaData.Response(HttpVersion.HTTP_2, HttpStatus.OK_200, new HttpFields()); + HeadersFrame responseFrame = new HeadersFrame(stream.getId(), response, null, true); + stream.headers(responseFrame, Callback.NOOP); + } + } + }; + } + }); + + HttpRequest request = (HttpRequest)client.newRequest("localhost", connector.getLocalPort()); + HttpFields trailers = new HttpFields(); + request.trailers(() -> trailers); + DeferredContentProvider content = new DeferredContentProvider(); + request.content(content); + + CountDownLatch latch = new CountDownLatch(1); + request.send(result -> + { + assertTrue(result.isSucceeded()); + assertEquals(HttpStatus.OK_200, result.getResponse().getStatus()); + latch.countDown(); + }); + + // Send deferred content after a while. + Thread.sleep(1000); + content.close(); + + assertTrue(latch.await(5, TimeUnit.SECONDS)); + } } From e81fdbd7c7b7388b079edc8c2ec170fa1e7aba18 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Wed, 12 Jun 2019 13:38:45 -0500 Subject: [PATCH 3/6] Issue #3736 - Give better exception when using bad classloader impl Signed-off-by: Joakim Erdfelt --- .../main/java/org/eclipse/jetty/webapp/WebAppClassLoader.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebAppClassLoader.java b/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebAppClassLoader.java index 967b9e5feaa..e043e6fb797 100644 --- a/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebAppClassLoader.java +++ b/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebAppClassLoader.java @@ -498,7 +498,9 @@ public class WebAppClassLoader extends URLClassLoader implements ClassVisibility // Try the parent loader try { - parent_class = _parent.loadClass(name); + parent_class = _parent.loadClass(name); + if (parent_class == null) + throw new ClassNotFoundException("Bad ClassLoader: returned null for loadClass(" + name + ")"); // If the webapp is allowed to see this class if (Boolean.TRUE.equals(__loadServerClasses.get()) || !_context.isServerClass(parent_class)) From 9e368726d76802e5e256250c4bc5e557249a1f97 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Wed, 12 Jun 2019 15:10:22 -0500 Subject: [PATCH 4/6] Attempting to address stats difference --- .../websocket/tests/WebSocketConnectionStatsTest.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/WebSocketConnectionStatsTest.java b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/WebSocketConnectionStatsTest.java index 97ca787eba5..68b16d1d909 100644 --- a/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/WebSocketConnectionStatsTest.java +++ b/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/WebSocketConnectionStatsTest.java @@ -33,7 +33,9 @@ import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.ServerConnector; import org.eclipse.jetty.servlet.ServletContextHandler; import org.eclipse.jetty.util.BufferUtil; +import org.eclipse.jetty.websocket.api.CloseStatus; import org.eclipse.jetty.websocket.api.Session; +import org.eclipse.jetty.websocket.api.StatusCode; import org.eclipse.jetty.websocket.api.WebSocketPolicy; import org.eclipse.jetty.websocket.api.annotations.OnWebSocketClose; import org.eclipse.jetty.websocket.api.annotations.OnWebSocketConnect; @@ -193,9 +195,11 @@ public class WebSocketConnectionStatsTest { session.getRemote().sendString(msgText); } + session.close(StatusCode.NORMAL, null); + + assertTrue(socket.closed.await(5, TimeUnit.SECONDS)); + assertTrue(wsConnectionClosed.await(5, TimeUnit.SECONDS)); } - assertTrue(socket.closed.await(5, TimeUnit.SECONDS)); - assertTrue(wsConnectionClosed.await(5, TimeUnit.SECONDS)); assertThat(statistics.getConnectionsMax(), is(1L)); assertThat(statistics.getConnections(), is(0L)); From 370a85b921ecbeb244c506cb3fdb5c7d8cd5ed09 Mon Sep 17 00:00:00 2001 From: Olivier Lamy Date: Thu, 13 Jun 2019 11:28:22 +1000 Subject: [PATCH 5/6] Jetty 9.4.x aggregate javadoc fix (#3780) * fix javadoc plugin configuration fixing searchbox when navigating to class found, as jetty 10 has module defined we probably do not need to merge this to 10 except other parameters Signed-off-by: olivier lamy --- pom.xml | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/pom.xml b/pom.xml index 0c6dd675e32..ad9030f57c0 100644 --- a/pom.xml +++ b/pom.xml @@ -283,7 +283,7 @@ true false javadoc:aggregate-jar deploy - -Peclipse-release -Paggregate-javadoc-jar + -Peclipse-release clean install forked-path @@ -534,20 +534,18 @@ org.apache.maven.plugins maven-javadoc-plugin - 3.0.1 + 3.1.0 8 - 8 UTF-8 UTF-8 UTF-8 - -html5 true false false protected true - com.acme,org.slf4j*,org.mortbay*,*.jmh*,org.eclipse.jetty.embedded*,org.eclipse.jetty.example.asyncrest*,org.eclipse.jetty.test* + com.*:org.slf4j*:org.mortbay*:*.jmh*:org.eclipse.jetty.embedded*:org.eclipse.jetty.example.asyncrest*:org.eclipse.jetty.test* @@ -1128,6 +1126,27 @@ + + jdk11 + + [11,) + + + + + + org.apache.maven.plugins + maven-javadoc-plugin + + --no-module-directories + -html5 + com.*:org.slf4j*:org.mortbay*:*.jmh*:org.eclipse.jetty.embedded*:org.eclipse.jetty.example.asyncrest*:org.eclipse.jetty.test* + + + + + + config From c94753ece3f79715c5d0707c60dcdfed17d678c9 Mon Sep 17 00:00:00 2001 From: Lachlan Date: Thu, 13 Jun 2019 15:51:22 +1000 Subject: [PATCH 6/6] Issue #3746 - fix WriteFlusher ClassCastException if Callback throws (#3778) Signed-off-by: Lachlan Roberts --- .../org/eclipse/jetty/io/WriteFlusher.java | 31 ++++++++++- .../eclipse/jetty/io/WriteFlusherTest.java | 55 ++++++++++++++++--- 2 files changed, 75 insertions(+), 11 deletions(-) diff --git a/jetty-io/src/main/java/org/eclipse/jetty/io/WriteFlusher.java b/jetty-io/src/main/java/org/eclipse/jetty/io/WriteFlusher.java index 9359e38f7d2..e55263c1954 100644 --- a/jetty-io/src/main/java/org/eclipse/jetty/io/WriteFlusher.java +++ b/jetty-io/src/main/java/org/eclipse/jetty/io/WriteFlusher.java @@ -307,9 +307,36 @@ abstract public class WriteFlusher private void fail(Callback callback, Throwable... suppressed) { - FailedState failed = (FailedState)_state.get(); + Throwable cause; + loop: + while (true) + { + State state = _state.get(); + + switch (state.getType()) + { + case FAILED: + { + FailedState failed = (FailedState)state; + cause = failed.getCause(); + break loop; + } + + case IDLE: + for (Throwable t : suppressed) + LOG.warn(t); + return; + + default: + Throwable t = new IllegalStateException(); + if (!_state.compareAndSet(state, new FailedState(t))) + continue; + + cause = t; + break loop; + } + } - Throwable cause = failed.getCause(); for (Throwable t : suppressed) { if (t != cause) diff --git a/jetty-io/src/test/java/org/eclipse/jetty/io/WriteFlusherTest.java b/jetty-io/src/test/java/org/eclipse/jetty/io/WriteFlusherTest.java index 185bc0a783e..f1d6aeb2b32 100644 --- a/jetty-io/src/test/java/org/eclipse/jetty/io/WriteFlusherTest.java +++ b/jetty-io/src/test/java/org/eclipse/jetty/io/WriteFlusherTest.java @@ -18,14 +18,6 @@ package org.eclipse.jetty.io; -import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.instanceOf; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.junit.jupiter.api.Assertions.assertTrue; - import java.io.IOException; import java.io.InterruptedIOException; import java.nio.ByteBuffer; @@ -43,10 +35,18 @@ import java.util.concurrent.atomic.AtomicBoolean; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.FutureCallback; +import org.eclipse.jetty.util.log.StacklessLogging; import org.hamcrest.Matchers; - import org.junit.jupiter.api.Test; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.instanceOf; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + public class WriteFlusherTest { @Test @@ -159,6 +159,43 @@ public class WriteFlusherTest assertTrue(flusher.isIdle()); } + @Test + public void testCallbackThrows() throws Exception + { + ByteArrayEndPoint endPoint = new ByteArrayEndPoint(new byte[0], 100); + + AtomicBoolean incompleteFlush = new AtomicBoolean(false); + WriteFlusher flusher = new WriteFlusher(endPoint) + { + @Override + protected void onIncompleteFlush() + { + incompleteFlush.set(true); + } + }; + + FutureCallback callback = new FutureCallback() + { + @Override + public void succeeded() + { + super.succeeded(); + throw new IllegalStateException(); + } + }; + + try (StacklessLogging stacklessLogging = new StacklessLogging(WriteFlusher.class)) + { + flusher.write(callback, BufferUtil.toBuffer("How now brown cow!")); + callback.get(100, TimeUnit.MILLISECONDS); + } + + assertEquals("How now brown cow!", endPoint.takeOutputString()); + assertTrue(callback.isDone()); + assertFalse(incompleteFlush.get()); + assertTrue(flusher.isIdle()); + } + @Test public void testCloseWhileBlocking() throws Exception {