From 9cc7be4842e96b995a29c2ab0c897969887f9568 Mon Sep 17 00:00:00 2001 From: gregw Date: Tue, 2 Feb 2021 14:03:38 +0100 Subject: [PATCH 01/33] Fix #5605 Unblock non container Threads Ensure that HttpInput is always closed to EOF, EarlyEOF or Error, so that non container threads doing blocking reads will not block forever, even if late. Delay recycling of HttpInput until next request is received. --- .../eclipse/jetty/io/AbstractEndPoint.java | 7 + .../java/org/eclipse/jetty/io/EndPoint.java | 3 + .../org/eclipse/jetty/io/FillInterest.java | 189 +++++++-- .../eclipse/jetty/server/HttpConnection.java | 5 +- .../org/eclipse/jetty/server/HttpInput.java | 14 + .../jetty/server/ProxyConnectionFactory.java | 7 + .../org/eclipse/jetty/server/Request.java | 4 +- .../eclipse/jetty/server/BlockingTest.java | 398 ++++++++++++++++++ 8 files changed, 590 insertions(+), 37 deletions(-) create mode 100644 jetty-server/src/test/java/org/eclipse/jetty/server/BlockingTest.java diff --git a/jetty-io/src/main/java/org/eclipse/jetty/io/AbstractEndPoint.java b/jetty-io/src/main/java/org/eclipse/jetty/io/AbstractEndPoint.java index 99237f853a78..c6635f732b0b 100644 --- a/jetty-io/src/main/java/org/eclipse/jetty/io/AbstractEndPoint.java +++ b/jetty-io/src/main/java/org/eclipse/jetty/io/AbstractEndPoint.java @@ -22,6 +22,7 @@ import java.nio.ByteBuffer; import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Supplier; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.Callback; @@ -362,6 +363,12 @@ public void fillInterested(Callback callback) _fillInterest.register(callback); } + @Override + public Throwable cancelFillInterest(Supplier cancellation) + { + return _fillInterest.cancel(cancellation); + } + @Override public boolean tryFillInterested(Callback callback) { diff --git a/jetty-io/src/main/java/org/eclipse/jetty/io/EndPoint.java b/jetty-io/src/main/java/org/eclipse/jetty/io/EndPoint.java index 16776dacbd32..99d4f3def3ab 100644 --- a/jetty-io/src/main/java/org/eclipse/jetty/io/EndPoint.java +++ b/jetty-io/src/main/java/org/eclipse/jetty/io/EndPoint.java @@ -24,6 +24,7 @@ import java.nio.ByteBuffer; import java.nio.channels.ReadPendingException; import java.nio.channels.WritePendingException; +import java.util.function.Supplier; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.FutureCallback; @@ -223,6 +224,8 @@ public interface EndPoint extends Closeable */ boolean isFillInterested(); + Throwable cancelFillInterest(Supplier cancellation); + /** *

Writes the given buffers via {@link #flush(ByteBuffer...)} and invokes callback methods when either * all the data has been flushed or an error occurs.

diff --git a/jetty-io/src/main/java/org/eclipse/jetty/io/FillInterest.java b/jetty-io/src/main/java/org/eclipse/jetty/io/FillInterest.java index fa7fc5fc88cc..967282768f87 100644 --- a/jetty-io/src/main/java/org/eclipse/jetty/io/FillInterest.java +++ b/jetty-io/src/main/java/org/eclipse/jetty/io/FillInterest.java @@ -21,7 +21,9 @@ import java.io.IOException; import java.nio.channels.ClosedChannelException; import java.nio.channels.ReadPendingException; +import java.util.concurrent.CancellationException; import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Supplier; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.log.Log; @@ -42,6 +44,38 @@ protected FillInterest() { } + /** + * Cancel a fill interest registration. + * + * If there was a registration, then any {@link #fillable()}, {@link #onClose()} or {@link #onFail(Throwable)} + * calls are remembered and passed to the next registration. + * Since any actions resulting from a call to {@link #needsFillInterest()} cannot be unwound, a subsequent call to + * register will not call {@link #needsFillInterest()} again if it has already been called an no callback received. + * @param cancellation A supplier of the cancellation Throwable to use if there is an existing registration. If the + * suppler or the supplied Throwable is null, then a new {@link CancellationException} is used. + * @return The Throwable used to cancel an existing registration or null if there was no registration to cancel. + */ + public Throwable cancel(Supplier cancellation) + { + Cancelled cancelled = new Cancelled(); + while (true) + { + Callback callback = _interested.get(); + if (callback == null || callback instanceof Cancelled) + return null; + if (_interested.compareAndSet(callback, cancelled)) + { + Throwable cause = cancellation == null ? null : cancellation.get(); + if (cause == null) + cause = new CancellationException(); + if (LOG.isDebugEnabled()) + LOG.debug("cancelled {} {}",this, callback, cause); + callback.failed(cause); + return cause; + } + } + } + /** * Call to register interest in a callback when a read is possible. * The callback will be called either immediately if {@link #needsFillInterest()} @@ -68,26 +102,63 @@ public void register(Callback callback) throws ReadPendingException * @return true if the register succeeded */ public boolean tryRegister(Callback callback) + { + return register(callback, null); + } + + /** + * Call to register interest in a callback when a read is possible. + * The callback will be called either immediately if {@link #needsFillInterest()} + * returns true or eventually once {@link #fillable()} is called. + * + * @param callback the callback to register + * @param cancellation A supplier of a {@link Throwable}, which if not null will be used to fail any existing registration + * @return true if the register succeeded + */ + public boolean register(Callback callback, Supplier cancellation) { if (callback == null) throw new IllegalArgumentException(); - if (!_interested.compareAndSet(null, callback)) - return false; + while (true) + { + Callback existing = _interested.get(); - if (LOG.isDebugEnabled()) - LOG.debug("interested {}", this); + if (existing != null && !(existing instanceof Cancelled) && cancellation == null) + return false; - try - { - needsFillInterest(); - } - catch (Throwable e) - { - onFail(e); - } + if (existing == callback) + return true; - return true; + if (_interested.compareAndSet(existing, callback)) + { + if (LOG.isDebugEnabled()) + LOG.debug("interested {}->{}", existing, this); + if (existing == null) + { + try + { + needsFillInterest(); + } + catch (Throwable e) + { + onFail(e); + } + } + else if (existing instanceof Cancelled) + { + ((Cancelled)existing).apply(callback); + } + else + { + Throwable cause = cancellation.get(); + if (cause == null) + cause = new CancellationException(); + existing.failed(cause); + } + return true; + } + } } /** @@ -97,17 +168,19 @@ public boolean tryRegister(Callback callback) */ public boolean fillable() { - if (LOG.isDebugEnabled()) - LOG.debug("fillable {}", this); - Callback callback = _interested.get(); - if (callback != null && _interested.compareAndSet(callback, null)) + while (true) { - callback.succeeded(); - return true; + Callback callback = _interested.get(); + if (callback == null) + return false; + if (_interested.compareAndSet(callback, null)) + { + if (LOG.isDebugEnabled()) + LOG.debug("fillable {} {}",this, callback); + callback.succeeded(); + return true; + } } - if (LOG.isDebugEnabled()) - LOG.debug("{} lost race {}", this, callback); - return false; } /** @@ -115,7 +188,8 @@ public boolean fillable() */ public boolean isInterested() { - return _interested.get() != null; + Callback callback = _interested.get(); + return callback != null && !(callback instanceof Cancelled); } public InvocationType getCallbackInvocationType() @@ -132,24 +206,37 @@ public InvocationType getCallbackInvocationType() */ public boolean onFail(Throwable cause) { - if (LOG.isDebugEnabled()) - LOG.debug("onFail " + this, cause); - Callback callback = _interested.get(); - if (callback != null && _interested.compareAndSet(callback, null)) + while (true) { - callback.failed(cause); - return true; + Callback callback = _interested.get(); + if (callback == null) + return false; + if (_interested.compareAndSet(callback, null)) + { + if (LOG.isDebugEnabled()) + LOG.debug("onFail {} {}",this, callback, cause); + callback.failed(cause); + return true; + } } - return false; } public void onClose() { - if (LOG.isDebugEnabled()) - LOG.debug("onClose {}", this); - Callback callback = _interested.get(); - if (callback != null && _interested.compareAndSet(callback, null)) - callback.failed(new ClosedChannelException()); + while (true) + { + Callback callback = _interested.get(); + if (callback == null) + return; + if (_interested.compareAndSet(callback, null)) + { + ClosedChannelException cause = new ClosedChannelException(); + if (LOG.isDebugEnabled()) + LOG.debug("onFail {} {}",this, callback, cause); + callback.failed(cause); + return; + } + } } @Override @@ -171,4 +258,36 @@ public String toStateString() * @throws IOException if unable to fulfill interest in fill */ protected abstract void needsFillInterest() throws IOException; + + private static class Cancelled implements Callback + { + private final AtomicReference _result = new AtomicReference<>(); + + @Override + public void succeeded() + { + _result.compareAndSet(null, Boolean.TRUE); + } + + @Override + public void failed(Throwable x) + { + _result.compareAndSet(null, x == null ? new Exception() : x); + } + + @Override + public InvocationType getInvocationType() + { + return InvocationType.NON_BLOCKING; + } + + void apply(Callback callback) + { + Object result = _result.get(); + if (result == Boolean.TRUE) + callback.succeeded(); + else if (result instanceof Throwable) + callback.failed((Throwable)result); + } + } } 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 3ae8bbe4bc67..7b503913b5bb 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 @@ -376,6 +376,9 @@ private boolean parseRequestBuffer() @Override public void onCompleted() { + boolean complete = _input.consumeAll(); + getEndPoint().cancelFillInterest(_input::getError); + // Handle connection upgrades if (_channel.getResponse().getStatus() == HttpStatus.SWITCHING_PROTOCOLS_101) { @@ -409,7 +412,7 @@ public void onCompleted() _parser.close(); } // else abort if we can't consume all - else if (_generator.isPersistent() && !_input.consumeAll()) + else if (_generator.isPersistent() && !complete) { if (LOG.isDebugEnabled()) LOG.debug("unconsumed input {} {}", this, _parser); 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 600260de34ad..724132c3e303 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 @@ -720,12 +720,17 @@ public boolean consumeAll() { produceContent(); if (_content == null && _intercepted == null && _inputQ.isEmpty()) + { + _state = EARLY_EOF; + _inputQ.notify(); return false; + } } catch (Throwable e) { LOG.debug(e); _state = new ErrorState(e); + _inputQ.notify(); return false; } } @@ -740,6 +745,15 @@ public boolean isError() } } + public Throwable getError() + { + synchronized (_inputQ) + { + Throwable error = _state instanceof ErrorState ? ((ErrorState)_state)._error : null; + return error == null ? new IOException() : error; + } + } + public boolean isAsync() { synchronized (_inputQ) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/ProxyConnectionFactory.java b/jetty-server/src/main/java/org/eclipse/jetty/server/ProxyConnectionFactory.java index 71cd03c274db..3938c0f272a6 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/ProxyConnectionFactory.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/ProxyConnectionFactory.java @@ -27,6 +27,7 @@ import java.nio.channels.ReadPendingException; import java.nio.channels.WritePendingException; import java.nio.charset.StandardCharsets; +import java.util.function.Supplier; import org.eclipse.jetty.io.AbstractConnection; import org.eclipse.jetty.io.Connection; @@ -805,6 +806,12 @@ public void fillInterested(Callback callback) throws ReadPendingException _endp.fillInterested(callback); } + @Override + public Throwable cancelFillInterest(Supplier cancellation) + { + return _endp.cancelFillInterest(cancellation); + } + @Override public boolean flush(ByteBuffer... buffer) throws IOException { diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java index d1c27ef2a72b..3cc4946c2d2e 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java @@ -1811,6 +1811,8 @@ public boolean isUserInRole(String role) */ public void setMetaData(org.eclipse.jetty.http.MetaData.Request request) { + if (_metaData == null) + _input.recycle(); _metaData = request; setMethod(request.getMethod()); @@ -1879,7 +1881,7 @@ protected void recycle() getHttpChannelState().recycle(); _requestAttributeListeners.clear(); - _input.recycle(); + // Defer _input.recycle() until setMetaData on next request, so that late readers will fail _metaData = null; _originalURI = null; _contextPath = null; diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingTest.java new file mode 100644 index 000000000000..62605a29dee5 --- /dev/null +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingTest.java @@ -0,0 +1,398 @@ +// +// ======================================================================== +// Copyright (c) 1995-2021 Mort Bay Consulting Pty Ltd and others. +// ------------------------------------------------------------------------ +// 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.server; + +import java.io.IOException; +import java.io.OutputStream; +import java.net.Socket; +import java.nio.charset.StandardCharsets; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; +import javax.servlet.AsyncContext; +import javax.servlet.DispatcherType; +import javax.servlet.ServletException; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import org.eclipse.jetty.http.HttpTester; +import org.eclipse.jetty.server.handler.AbstractHandler; +import org.eclipse.jetty.server.handler.ContextHandler; +import org.eclipse.jetty.server.handler.DefaultHandler; +import org.eclipse.jetty.server.handler.HandlerList; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +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.hamcrest.Matchers.notNullValue; +import static org.hamcrest.core.Is.is; +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class BlockingTest +{ + private Server server; + ServerConnector connector; + private ContextHandler context; + + @BeforeEach + void setUp() + { + server = new Server(); + connector = new ServerConnector(server); + connector.setPort(0); + server.addConnector(connector); + + context = new ContextHandler("/ctx"); + + HandlerList handlers = new HandlerList(); + handlers.setHandlers(new Handler[]{context, new DefaultHandler()}); + server.setHandler(handlers); + } + + @AfterEach + void tearDown() throws Exception + { + server.stop(); + } + + @Test + public void testBlockingReadThenNormalComplete() throws Exception + { + CountDownLatch started = new CountDownLatch(1); + CountDownLatch stopped = new CountDownLatch(1); + AtomicReference readException = new AtomicReference<>(); + AbstractHandler handler = new AbstractHandler() + { + @Override + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + { + baseRequest.setHandled(true); + new Thread(() -> + { + try + { + int b = baseRequest.getHttpInput().read(); + if (b == '1') + { + started.countDown(); + if (baseRequest.getHttpInput().read() > Integer.MIN_VALUE) + throw new IllegalStateException(); + } + } + catch (Throwable t) + { + readException.set(t); + stopped.countDown(); + } + }).start(); + + try + { + // wait for thread to start and read first byte + started.await(10, TimeUnit.SECONDS); + // give it time to block on second byte + Thread.sleep(1000); + } + catch (Throwable e) + { + throw new ServletException(e); + } + + response.setStatus(200); + response.setContentType("text/plain"); + response.getOutputStream().print("OK\r\n"); + } + }; + context.setHandler(handler); + server.start(); + + StringBuilder request = new StringBuilder(); + request.append("POST /ctx/path/info HTTP/1.1\r\n") + .append("Host: localhost\r\n") + .append("Content-Type: test/data\r\n") + .append("Content-Length: 2\r\n") + .append("\r\n") + .append("1"); + + int port = connector.getLocalPort(); + try (Socket socket = new Socket("localhost", port)) + { + socket.setSoTimeout(1000000); + OutputStream out = socket.getOutputStream(); + out.write(request.toString().getBytes(StandardCharsets.ISO_8859_1)); + + HttpTester.Response response = HttpTester.parseResponse(socket.getInputStream()); + assertThat(response, notNullValue()); + assertThat(response.getStatus(), is(200)); + assertThat(response.getContent(), containsString("OK")); + + // Async thread should have stopped + assertTrue(stopped.await(10, TimeUnit.SECONDS)); + assertThat(readException.get(), instanceOf(IOException.class)); + } + } + + @Test + public void testNormalCompleteThenBlockingRead() throws Exception + { + CountDownLatch started = new CountDownLatch(1); + CountDownLatch completed = new CountDownLatch(1); + CountDownLatch stopped = new CountDownLatch(1); + AtomicReference readException = new AtomicReference<>(); + AbstractHandler handler = new AbstractHandler() + { + @Override + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + { + baseRequest.setHandled(true); + new Thread(() -> + { + try + { + int b = baseRequest.getHttpInput().read(); + if (b == '1') + { + started.countDown(); + completed.await(10, TimeUnit.SECONDS); + Thread.sleep(500); + if (baseRequest.getHttpInput().read() > Integer.MIN_VALUE) + throw new IllegalStateException(); + } + } + catch (Throwable t) + { + readException.set(t); + stopped.countDown(); + } + }).start(); + + try + { + // wait for thread to start and read first byte + started.await(10, TimeUnit.SECONDS); + // give it time to block on second byte + Thread.sleep(1000); + } + catch (Throwable e) + { + throw new ServletException(e); + } + + response.setStatus(200); + response.setContentType("text/plain"); + response.getOutputStream().print("OK\r\n"); + } + }; + context.setHandler(handler); + server.start(); + + StringBuilder request = new StringBuilder(); + request.append("POST /ctx/path/info HTTP/1.1\r\n") + .append("Host: localhost\r\n") + .append("Content-Type: test/data\r\n") + .append("Content-Length: 2\r\n") + .append("\r\n") + .append("1"); + + int port = connector.getLocalPort(); + try (Socket socket = new Socket("localhost", port)) + { + socket.setSoTimeout(1000000); + OutputStream out = socket.getOutputStream(); + out.write(request.toString().getBytes(StandardCharsets.ISO_8859_1)); + + HttpTester.Response response = HttpTester.parseResponse(socket.getInputStream()); + assertThat(response, notNullValue()); + assertThat(response.getStatus(), is(200)); + assertThat(response.getContent(), containsString("OK")); + + completed.countDown(); + Thread.sleep(1000); + + // Async thread should have stopped + assertTrue(stopped.await(10, TimeUnit.SECONDS)); + assertThat(readException.get(), instanceOf(IOException.class)); + } + } + + @Test + public void testStartAsyncThenBlockingReadThenTimeout() throws Exception + { + CountDownLatch started = new CountDownLatch(1); + CountDownLatch completed = new CountDownLatch(1); + CountDownLatch stopped = new CountDownLatch(1); + AtomicReference readException = new AtomicReference<>(); + AbstractHandler handler = new AbstractHandler() + { + @Override + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws ServletException + { + baseRequest.setHandled(true); + if (baseRequest.getDispatcherType() != DispatcherType.ERROR) + { + AsyncContext async = request.startAsync(); + async.setTimeout(100); + + new Thread(() -> + { + try + { + int b = baseRequest.getHttpInput().read(); + if (b == '1') + { + started.countDown(); + completed.await(10, TimeUnit.SECONDS); + Thread.sleep(500); + if (baseRequest.getHttpInput().read() > Integer.MIN_VALUE) + throw new IllegalStateException(); + } + } + catch (Throwable t) + { + readException.set(t); + stopped.countDown(); + } + }).start(); + + try + { + // wait for thread to start and read first byte + started.await(10, TimeUnit.SECONDS); + // give it time to block on second byte + Thread.sleep(1000); + } + catch (Throwable e) + { + throw new ServletException(e); + } + } + } + }; + context.setHandler(handler); + server.start(); + + StringBuilder request = new StringBuilder(); + request.append("POST /ctx/path/info HTTP/1.1\r\n") + .append("Host: localhost\r\n") + .append("Content-Type: test/data\r\n") + .append("Content-Length: 2\r\n") + .append("\r\n") + .append("1"); + + int port = connector.getLocalPort(); + try (Socket socket = new Socket("localhost", port)) + { + socket.setSoTimeout(1000000); + OutputStream out = socket.getOutputStream(); + out.write(request.toString().getBytes(StandardCharsets.ISO_8859_1)); + + HttpTester.Response response = HttpTester.parseResponse(socket.getInputStream()); + assertThat(response, notNullValue()); + assertThat(response.getStatus(), is(500)); + assertThat(response.getContent(), containsString("AsyncContext timeout")); + + completed.countDown(); + Thread.sleep(1000); + + // Async thread should have stopped + assertTrue(stopped.await(10, TimeUnit.SECONDS)); + assertThat(readException.get(), instanceOf(IOException.class)); + } + } + + @Test + public void testBlockingReadThenSendError() throws Exception + { + CountDownLatch started = new CountDownLatch(1); + CountDownLatch stopped = new CountDownLatch(1); + AtomicReference readException = new AtomicReference<>(); + AbstractHandler handler = new AbstractHandler() + { + @Override + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + { + baseRequest.setHandled(true); + if (baseRequest.getDispatcherType() != DispatcherType.ERROR) + { + new Thread(() -> + { + try + { + int b = baseRequest.getHttpInput().read(); + if (b == '1') + { + started.countDown(); + if (baseRequest.getHttpInput().read() > Integer.MIN_VALUE) + throw new IllegalStateException(); + } + } + catch (Throwable t) + { + readException.set(t); + stopped.countDown(); + } + }).start(); + + try + { + // wait for thread to start and read first byte + started.await(10, TimeUnit.SECONDS); + // give it time to block on second byte + Thread.sleep(1000); + } + catch (Throwable e) + { + throw new ServletException(e); + } + + response.sendError(499); + } + } + }; + context.setHandler(handler); + server.start(); + + StringBuilder request = new StringBuilder(); + request.append("POST /ctx/path/info HTTP/1.1\r\n") + .append("Host: localhost\r\n") + .append("Content-Type: test/data\r\n") + .append("Content-Length: 2\r\n") + .append("\r\n") + .append("1"); + + int port = connector.getLocalPort(); + try (Socket socket = new Socket("localhost", port)) + { + socket.setSoTimeout(1000000); + OutputStream out = socket.getOutputStream(); + out.write(request.toString().getBytes(StandardCharsets.ISO_8859_1)); + + HttpTester.Response response = HttpTester.parseResponse(socket.getInputStream()); + assertThat(response, notNullValue()); + assertThat(response.getStatus(), is(499)); + + // Async thread should have stopped + assertTrue(stopped.await(10, TimeUnit.SECONDS)); + assertThat(readException.get(), instanceOf(IOException.class)); + } + } +} From b3268eb3b584666a50dca08dad80893d1a69620d Mon Sep 17 00:00:00 2001 From: gregw Date: Tue, 2 Feb 2021 16:33:32 +0100 Subject: [PATCH 02/33] Fix #5605 Unblock non container Threads Ensure that HttpInput is always closed to EOF, EarlyEOF or Error, so that non container threads doing blocking reads will not block forever, even if late. Delay recycling of HttpInput until next request is received. --- .../eclipse/jetty/websocket/common/io/MockEndPoint.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/jetty-websocket/websocket-common/src/test/java/org/eclipse/jetty/websocket/common/io/MockEndPoint.java b/jetty-websocket/websocket-common/src/test/java/org/eclipse/jetty/websocket/common/io/MockEndPoint.java index b57c0f9bff9e..8a541ea10aee 100644 --- a/jetty-websocket/websocket-common/src/test/java/org/eclipse/jetty/websocket/common/io/MockEndPoint.java +++ b/jetty-websocket/websocket-common/src/test/java/org/eclipse/jetty/websocket/common/io/MockEndPoint.java @@ -23,6 +23,7 @@ import java.nio.ByteBuffer; import java.nio.channels.ReadPendingException; import java.nio.channels.WritePendingException; +import java.util.function.Supplier; import org.eclipse.jetty.io.Connection; import org.eclipse.jetty.io.EndPoint; @@ -116,6 +117,12 @@ public void fillInterested(Callback callback) throws ReadPendingException throw new UnsupportedOperationException(NOT_SUPPORTED); } + @Override + public Throwable cancelFillInterest(Supplier cancellation) + { + throw new UnsupportedOperationException(NOT_SUPPORTED); + } + @Override public boolean tryFillInterested(Callback callback) { From 0d85c7d2200fd5c495205234907cb9aad70c14de Mon Sep 17 00:00:00 2001 From: gregw Date: Tue, 2 Feb 2021 17:49:17 +0100 Subject: [PATCH 03/33] Fix #5605 Unblock non container Threads test and fixes for the write side. --- .../eclipse/jetty/server/HttpConnection.java | 4 +- .../org/eclipse/jetty/server/HttpOutput.java | 12 +- .../eclipse/jetty/server/BlockingTest.java | 106 ++++++++++++++++++ .../jetty/util/SharedBlockingCallback.java | 39 ++++++- 4 files changed, 155 insertions(+), 6 deletions(-) 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 7b503913b5bb..78f96cf9ccbd 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 @@ -377,7 +377,9 @@ private boolean parseRequestBuffer() public void onCompleted() { boolean complete = _input.consumeAll(); - getEndPoint().cancelFillInterest(_input::getError); + Throwable cancelled = getEndPoint().cancelFillInterest(_input::getError); + if (LOG.isDebugEnabled()) + LOG.debug("cancelled {}", this, cancelled); // Handle connection upgrades if (_channel.getResponse().getStatus() == HttpStatus.SWITCHING_PROTOCOLS_101) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java index 799fa5fe3395..143931091b7c 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java @@ -29,6 +29,7 @@ import java.nio.charset.CoderResult; import java.nio.charset.CodingErrorAction; import java.util.ResourceBundle; +import java.util.concurrent.CancellationException; import java.util.concurrent.TimeUnit; import javax.servlet.RequestDispatcher; import javax.servlet.ServletOutputStream; @@ -449,6 +450,15 @@ public void complete(Callback callback) break; case BLOCKED: + CancellationException cancelled = new CancellationException(); + if (_writeBlocker.fail(cancelled)) + _channel.abort(cancelled); + // An operation is in progress, so we soft close now + _softClose = true; + // then trigger a close from onWriteComplete + _state = State.CLOSE; + break; + case UNREADY: case PENDING: // An operation is in progress, so we soft close now @@ -1399,7 +1409,7 @@ public void recycle() { _state = State.OPEN; _apiState = ApiState.BLOCKING; - _softClose = false; + _softClose = true; // Stay closed until next request _interceptor = _channel; HttpConfiguration config = _channel.getHttpConfiguration(); _bufferSize = config.getOutputBufferSize(); diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingTest.java index 62605a29dee5..9a2f20b38287 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingTest.java @@ -18,10 +18,15 @@ package org.eclipse.jetty.server; +import java.io.BufferedReader; import java.io.IOException; +import java.io.InputStreamReader; import java.io.OutputStream; import java.net.Socket; import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; @@ -44,6 +49,7 @@ import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.notNullValue; +import static org.hamcrest.Matchers.startsWith; import static org.hamcrest.core.Is.is; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -395,4 +401,104 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques assertThat(readException.get(), instanceOf(IOException.class)); } } + + @Test + public void testBlockingWriteThenNormalComplete() throws Exception + { + CountDownLatch started = new CountDownLatch(1); + CountDownLatch stopped = new CountDownLatch(1); + AtomicReference readException = new AtomicReference<>(); + AbstractHandler handler = new AbstractHandler() + { + @Override + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws ServletException + { + baseRequest.setHandled(true); + response.setStatus(200); + response.setContentType("text/plain"); + new Thread(() -> + { + try + { + byte[] data = new byte[16 * 1024]; + Arrays.fill(data, (byte)'X'); + data[data.length - 2] = '\r'; + data[data.length - 1] = '\n'; + OutputStream out = response.getOutputStream(); + started.countDown(); + while (true) + out.write(data); + } + catch (Throwable t) + { + readException.set(t); + stopped.countDown(); + } + }).start(); + + try + { + // wait for thread to start and read first byte + started.await(10, TimeUnit.SECONDS); + // give it time to block on write + Thread.sleep(1000); + } + catch (Throwable e) + { + throw new ServletException(e); + } + } + }; + context.setHandler(handler); + server.start(); + + StringBuilder request = new StringBuilder(); + request.append("GET /ctx/path/info HTTP/1.1\r\n") + .append("Host: localhost\r\n") + .append("\r\n"); + + int port = connector.getLocalPort(); + try (Socket socket = new Socket("localhost", port)) + { + socket.setSoTimeout(1000000); + OutputStream out = socket.getOutputStream(); + out.write(request.toString().getBytes(StandardCharsets.ISO_8859_1)); + + BufferedReader in = new BufferedReader(new InputStreamReader(socket.getInputStream(), StandardCharsets.ISO_8859_1)); + + // Read the header + List header = new ArrayList<>(); + while (true) + { + String line = in.readLine(); + if (line.length() == 0) + break; + header.add(line); + } + assertThat(header.get(0), containsString("200 OK")); + + // read one line of content + String content = in.readLine(); + assertThat(content, is("4000")); + content = in.readLine(); + assertThat(content, startsWith("XXXXXXXX")); + + // check that writing thread is stopped by end of request handling + assertTrue(stopped.await(10, TimeUnit.SECONDS)); + + // read until last line + String last = null; + while (true) + { + String line = in.readLine(); + if (line == null) + break; + + last = line; + } + + // last line is not empty chunk, ie abnormal completion + assertThat(last, startsWith("XXXXX")); + } + } } diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/SharedBlockingCallback.java b/jetty-util/src/main/java/org/eclipse/jetty/util/SharedBlockingCallback.java index eea89c8ea248..daad08cc5029 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/SharedBlockingCallback.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/SharedBlockingCallback.java @@ -21,6 +21,7 @@ import java.io.Closeable; import java.io.IOException; import java.io.InterruptedIOException; +import java.util.Objects; import java.util.concurrent.CancellationException; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; @@ -50,10 +51,10 @@ public class SharedBlockingCallback { private static final Logger LOG = Log.getLogger(SharedBlockingCallback.class); - private static Throwable IDLE = new ConstantThrowable("IDLE"); - private static Throwable SUCCEEDED = new ConstantThrowable("SUCCEEDED"); + private static final Throwable IDLE = new ConstantThrowable("IDLE"); + private static final Throwable SUCCEEDED = new ConstantThrowable("SUCCEEDED"); - private static Throwable FAILED = new ConstantThrowable("FAILED"); + private static final Throwable FAILED = new ConstantThrowable("FAILED"); private final ReentrantLock _lock = new ReentrantLock(); private final Condition _idle = _lock.newCondition(); @@ -96,6 +97,26 @@ public Blocker acquire() throws IOException } } + public boolean fail(Throwable cause) + { + Objects.requireNonNull(cause); + _lock.lock(); + try + { + if (_blocker._state == null) + { + _blocker._state = new BlockerFailedException(cause); + _complete.signalAll(); + return true; + } + } + finally + { + _lock.unlock(); + } + return false; + } + protected void notComplete(Blocker blocker) { LOG.warn("Blocker not complete {}", blocker); @@ -165,10 +186,12 @@ else if (cause instanceof BlockerTimeoutException) _state = cause; _complete.signalAll(); } - else if (_state instanceof BlockerTimeoutException) + else if (_state instanceof BlockerTimeoutException || _state instanceof BlockerFailedException) { // Failure arrived late, block() already // modified the state, nothing more to do. + if (LOG.isDebugEnabled()) + LOG.debug("Failed after {}", _state); } else { @@ -297,4 +320,12 @@ public String toString() private static class BlockerTimeoutException extends TimeoutException { } + + private static class BlockerFailedException extends Exception + { + public BlockerFailedException(Throwable cause) + { + super(cause); + } + } } From a110fc3468041892b20590095f7d2bb0557186dc Mon Sep 17 00:00:00 2001 From: gregw Date: Tue, 2 Feb 2021 17:55:15 +0100 Subject: [PATCH 04/33] Fix #5605 Unblock non container Threads test and fixes for the write side. --- .../src/main/java/org/eclipse/jetty/server/Request.java | 3 +++ .../src/test/java/org/eclipse/jetty/server/ResponseTest.java | 2 ++ 2 files changed, 5 insertions(+) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java index 3cc4946c2d2e..bdb455694a78 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java @@ -1812,7 +1812,10 @@ public boolean isUserInRole(String role) public void setMetaData(org.eclipse.jetty.http.MetaData.Request request) { if (_metaData == null) + { _input.recycle(); + _channel.getResponse().getHttpOutput().reopen(); + } _metaData = request; setMethod(request.getMethod()); diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java index 51d8133c05b0..5ba0df395420 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java @@ -493,6 +493,7 @@ public void testContentTypeWithCharacterEncoding() throws Exception assertEquals("foo2/bar2;charset=utf-8", response.getContentType()); response.recycle(); + response.reopen(); response.setCharacterEncoding("utf16"); response.setContentType("text/html; charset=utf-8"); @@ -505,6 +506,7 @@ public void testContentTypeWithCharacterEncoding() throws Exception assertEquals("text/xml;charset=utf-8", response.getContentType()); response.recycle(); + response.reopen(); response.setCharacterEncoding("utf-16"); response.setContentType("foo/bar"); assertEquals("foo/bar;charset=utf-16", response.getContentType()); From a100d80d26dd4d97b8ad2bab5e32fff190c46feb Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 3 Feb 2021 11:03:02 +0100 Subject: [PATCH 05/33] Fix #5605 Unblock non container Threads Don't consumeAll before upgrade --- .../org/eclipse/jetty/server/HttpConnection.java | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) 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 78f96cf9ccbd..9b7bd630a326 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 @@ -376,17 +376,15 @@ private boolean parseRequestBuffer() @Override public void onCompleted() { - boolean complete = _input.consumeAll(); - Throwable cancelled = getEndPoint().cancelFillInterest(_input::getError); - if (LOG.isDebugEnabled()) - LOG.debug("cancelled {}", this, cancelled); - // Handle connection upgrades if (_channel.getResponse().getStatus() == HttpStatus.SWITCHING_PROTOCOLS_101) { Connection connection = (Connection)_channel.getRequest().getAttribute(UPGRADE_CONNECTION_ATTRIBUTE); if (connection != null) { + Throwable cancelled = getEndPoint().cancelFillInterest(_input::getError); + if (LOG.isDebugEnabled()) + LOG.debug("cancelled {}", this, cancelled); if (LOG.isDebugEnabled()) LOG.debug("Upgrade from {} to {}", this, connection); _channel.getState().upgrade(); @@ -406,6 +404,11 @@ public void onCompleted() } } + boolean complete = _input.consumeAll(); + Throwable cancelled = getEndPoint().cancelFillInterest(_input::getError); + if (LOG.isDebugEnabled()) + LOG.debug("cancelled {}", this, cancelled); + // Finish consuming the request // If we are still expecting if (_channel.isExpecting100Continue()) From 25cbe652a0bdd811289858dc0548a75faffc48cb Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 3 Feb 2021 11:27:25 +0100 Subject: [PATCH 06/33] Fix #5605 Unblock non container Threads reorder --- .../src/main/java/org/eclipse/jetty/server/HttpOutput.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java index 143931091b7c..248b85caccd0 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java @@ -450,11 +450,12 @@ public void complete(Callback callback) break; case BLOCKED: + // An operation is in progress, so we soft close now + _softClose = true; + // then cancel the operation CancellationException cancelled = new CancellationException(); if (_writeBlocker.fail(cancelled)) _channel.abort(cancelled); - // An operation is in progress, so we soft close now - _softClose = true; // then trigger a close from onWriteComplete _state = State.CLOSE; break; From 70056e2c690b6f92ca0d396123989ad2074a370b Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 3 Feb 2021 13:40:06 +0100 Subject: [PATCH 07/33] Fix #5605 Unblock non container Threads fix test --- .../src/main/java/org/eclipse/jetty/server/Request.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java index bdb455694a78..5eb360f376c5 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java @@ -1811,7 +1811,7 @@ public boolean isUserInRole(String role) */ public void setMetaData(org.eclipse.jetty.http.MetaData.Request request) { - if (_metaData == null) + if (_metaData == null && _input != null && _channel != null) { _input.recycle(); _channel.getResponse().getHttpOutput().reopen(); From 5f4919c45aa3bb6d232bbed479282b25da6dce8d Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 3 Feb 2021 14:04:29 +0100 Subject: [PATCH 08/33] Fix #5605 Unblock non container Threads cleanup debug --- .../main/java/org/eclipse/jetty/server/HttpConnection.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) 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 9b7bd630a326..758df92020f8 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 @@ -384,9 +384,7 @@ public void onCompleted() { Throwable cancelled = getEndPoint().cancelFillInterest(_input::getError); if (LOG.isDebugEnabled()) - LOG.debug("cancelled {}", this, cancelled); - if (LOG.isDebugEnabled()) - LOG.debug("Upgrade from {} to {}", this, connection); + LOG.debug("Upgrade from {} to {}", this, connection, cancelled); _channel.getState().upgrade(); getEndPoint().upgrade(connection); _channel.recycle(); From e9315fe51fbec49df96ba1e812c86653384698ae Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 3 Feb 2021 17:27:25 +0100 Subject: [PATCH 09/33] Fix #5605 Unblock non container Threads revert fillInterest cancellation and just abort connection instead. tested for all transports --- .../eclipse/jetty/io/AbstractEndPoint.java | 7 - .../java/org/eclipse/jetty/io/EndPoint.java | 3 - .../org/eclipse/jetty/io/FillInterest.java | 189 ++++-------------- .../eclipse/jetty/server/HttpConnection.java | 35 ++-- .../jetty/server/ProxyConnectionFactory.java | 7 - .../websocket/common/io/MockEndPoint.java | 7 - .../jetty/http/client/BlockedIOTest.java | 144 +++++++++++++ 7 files changed, 198 insertions(+), 194 deletions(-) create mode 100644 tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/BlockedIOTest.java diff --git a/jetty-io/src/main/java/org/eclipse/jetty/io/AbstractEndPoint.java b/jetty-io/src/main/java/org/eclipse/jetty/io/AbstractEndPoint.java index c6635f732b0b..99237f853a78 100644 --- a/jetty-io/src/main/java/org/eclipse/jetty/io/AbstractEndPoint.java +++ b/jetty-io/src/main/java/org/eclipse/jetty/io/AbstractEndPoint.java @@ -22,7 +22,6 @@ import java.nio.ByteBuffer; import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicReference; -import java.util.function.Supplier; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.Callback; @@ -363,12 +362,6 @@ public void fillInterested(Callback callback) _fillInterest.register(callback); } - @Override - public Throwable cancelFillInterest(Supplier cancellation) - { - return _fillInterest.cancel(cancellation); - } - @Override public boolean tryFillInterested(Callback callback) { diff --git a/jetty-io/src/main/java/org/eclipse/jetty/io/EndPoint.java b/jetty-io/src/main/java/org/eclipse/jetty/io/EndPoint.java index 99d4f3def3ab..16776dacbd32 100644 --- a/jetty-io/src/main/java/org/eclipse/jetty/io/EndPoint.java +++ b/jetty-io/src/main/java/org/eclipse/jetty/io/EndPoint.java @@ -24,7 +24,6 @@ import java.nio.ByteBuffer; import java.nio.channels.ReadPendingException; import java.nio.channels.WritePendingException; -import java.util.function.Supplier; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.FutureCallback; @@ -224,8 +223,6 @@ public interface EndPoint extends Closeable */ boolean isFillInterested(); - Throwable cancelFillInterest(Supplier cancellation); - /** *

Writes the given buffers via {@link #flush(ByteBuffer...)} and invokes callback methods when either * all the data has been flushed or an error occurs.

diff --git a/jetty-io/src/main/java/org/eclipse/jetty/io/FillInterest.java b/jetty-io/src/main/java/org/eclipse/jetty/io/FillInterest.java index 967282768f87..fa7fc5fc88cc 100644 --- a/jetty-io/src/main/java/org/eclipse/jetty/io/FillInterest.java +++ b/jetty-io/src/main/java/org/eclipse/jetty/io/FillInterest.java @@ -21,9 +21,7 @@ import java.io.IOException; import java.nio.channels.ClosedChannelException; import java.nio.channels.ReadPendingException; -import java.util.concurrent.CancellationException; import java.util.concurrent.atomic.AtomicReference; -import java.util.function.Supplier; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.log.Log; @@ -44,38 +42,6 @@ protected FillInterest() { } - /** - * Cancel a fill interest registration. - * - * If there was a registration, then any {@link #fillable()}, {@link #onClose()} or {@link #onFail(Throwable)} - * calls are remembered and passed to the next registration. - * Since any actions resulting from a call to {@link #needsFillInterest()} cannot be unwound, a subsequent call to - * register will not call {@link #needsFillInterest()} again if it has already been called an no callback received. - * @param cancellation A supplier of the cancellation Throwable to use if there is an existing registration. If the - * suppler or the supplied Throwable is null, then a new {@link CancellationException} is used. - * @return The Throwable used to cancel an existing registration or null if there was no registration to cancel. - */ - public Throwable cancel(Supplier cancellation) - { - Cancelled cancelled = new Cancelled(); - while (true) - { - Callback callback = _interested.get(); - if (callback == null || callback instanceof Cancelled) - return null; - if (_interested.compareAndSet(callback, cancelled)) - { - Throwable cause = cancellation == null ? null : cancellation.get(); - if (cause == null) - cause = new CancellationException(); - if (LOG.isDebugEnabled()) - LOG.debug("cancelled {} {}",this, callback, cause); - callback.failed(cause); - return cause; - } - } - } - /** * Call to register interest in a callback when a read is possible. * The callback will be called either immediately if {@link #needsFillInterest()} @@ -102,63 +68,26 @@ public void register(Callback callback) throws ReadPendingException * @return true if the register succeeded */ public boolean tryRegister(Callback callback) - { - return register(callback, null); - } - - /** - * Call to register interest in a callback when a read is possible. - * The callback will be called either immediately if {@link #needsFillInterest()} - * returns true or eventually once {@link #fillable()} is called. - * - * @param callback the callback to register - * @param cancellation A supplier of a {@link Throwable}, which if not null will be used to fail any existing registration - * @return true if the register succeeded - */ - public boolean register(Callback callback, Supplier cancellation) { if (callback == null) throw new IllegalArgumentException(); - while (true) - { - Callback existing = _interested.get(); - - if (existing != null && !(existing instanceof Cancelled) && cancellation == null) - return false; + if (!_interested.compareAndSet(null, callback)) + return false; - if (existing == callback) - return true; + if (LOG.isDebugEnabled()) + LOG.debug("interested {}", this); - if (_interested.compareAndSet(existing, callback)) - { - if (LOG.isDebugEnabled()) - LOG.debug("interested {}->{}", existing, this); - if (existing == null) - { - try - { - needsFillInterest(); - } - catch (Throwable e) - { - onFail(e); - } - } - else if (existing instanceof Cancelled) - { - ((Cancelled)existing).apply(callback); - } - else - { - Throwable cause = cancellation.get(); - if (cause == null) - cause = new CancellationException(); - existing.failed(cause); - } - return true; - } + try + { + needsFillInterest(); + } + catch (Throwable e) + { + onFail(e); } + + return true; } /** @@ -168,19 +97,17 @@ else if (existing instanceof Cancelled) */ public boolean fillable() { - while (true) + if (LOG.isDebugEnabled()) + LOG.debug("fillable {}", this); + Callback callback = _interested.get(); + if (callback != null && _interested.compareAndSet(callback, null)) { - Callback callback = _interested.get(); - if (callback == null) - return false; - if (_interested.compareAndSet(callback, null)) - { - if (LOG.isDebugEnabled()) - LOG.debug("fillable {} {}",this, callback); - callback.succeeded(); - return true; - } + callback.succeeded(); + return true; } + if (LOG.isDebugEnabled()) + LOG.debug("{} lost race {}", this, callback); + return false; } /** @@ -188,8 +115,7 @@ public boolean fillable() */ public boolean isInterested() { - Callback callback = _interested.get(); - return callback != null && !(callback instanceof Cancelled); + return _interested.get() != null; } public InvocationType getCallbackInvocationType() @@ -206,37 +132,24 @@ public InvocationType getCallbackInvocationType() */ public boolean onFail(Throwable cause) { - while (true) + if (LOG.isDebugEnabled()) + LOG.debug("onFail " + this, cause); + Callback callback = _interested.get(); + if (callback != null && _interested.compareAndSet(callback, null)) { - Callback callback = _interested.get(); - if (callback == null) - return false; - if (_interested.compareAndSet(callback, null)) - { - if (LOG.isDebugEnabled()) - LOG.debug("onFail {} {}",this, callback, cause); - callback.failed(cause); - return true; - } + callback.failed(cause); + return true; } + return false; } public void onClose() { - while (true) - { - Callback callback = _interested.get(); - if (callback == null) - return; - if (_interested.compareAndSet(callback, null)) - { - ClosedChannelException cause = new ClosedChannelException(); - if (LOG.isDebugEnabled()) - LOG.debug("onFail {} {}",this, callback, cause); - callback.failed(cause); - return; - } - } + if (LOG.isDebugEnabled()) + LOG.debug("onClose {}", this); + Callback callback = _interested.get(); + if (callback != null && _interested.compareAndSet(callback, null)) + callback.failed(new ClosedChannelException()); } @Override @@ -258,36 +171,4 @@ public String toStateString() * @throws IOException if unable to fulfill interest in fill */ protected abstract void needsFillInterest() throws IOException; - - private static class Cancelled implements Callback - { - private final AtomicReference _result = new AtomicReference<>(); - - @Override - public void succeeded() - { - _result.compareAndSet(null, Boolean.TRUE); - } - - @Override - public void failed(Throwable x) - { - _result.compareAndSet(null, x == null ? new Exception() : x); - } - - @Override - public InvocationType getInvocationType() - { - return InvocationType.NON_BLOCKING; - } - - void apply(Callback callback) - { - Object result = _result.get(); - if (result == Boolean.TRUE) - callback.succeeded(); - else if (result instanceof Throwable) - callback.failed((Throwable)result); - } - } } 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 758df92020f8..47caad8e43c0 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 @@ -382,30 +382,33 @@ public void onCompleted() Connection connection = (Connection)_channel.getRequest().getAttribute(UPGRADE_CONNECTION_ATTRIBUTE); if (connection != null) { - Throwable cancelled = getEndPoint().cancelFillInterest(_input::getError); - if (LOG.isDebugEnabled()) - LOG.debug("Upgrade from {} to {}", this, connection, cancelled); - _channel.getState().upgrade(); - getEndPoint().upgrade(connection); - _channel.recycle(); - _parser.reset(); - _generator.reset(); - if (_contentBufferReferences.get() == 0) - releaseRequestBuffer(); + if (isFillInterested()) + abort(new IllegalStateException()); else { - LOG.warn("{} lingering content references?!?!", this); - _requestBuffer = null; // Not returned to pool! - _contentBufferReferences.set(0); + if (LOG.isDebugEnabled()) + LOG.debug("Upgrade from {} to {}", this, connection); + _channel.getState().upgrade(); + getEndPoint().upgrade(connection); + _channel.recycle(); + _parser.reset(); + _generator.reset(); + if (_contentBufferReferences.get() == 0) + releaseRequestBuffer(); + else + { + LOG.warn("{} lingering content references?!?!", this); + _requestBuffer = null; // Not returned to pool! + _contentBufferReferences.set(0); + } } return; } } boolean complete = _input.consumeAll(); - Throwable cancelled = getEndPoint().cancelFillInterest(_input::getError); - if (LOG.isDebugEnabled()) - LOG.debug("cancelled {}", this, cancelled); + if (isFillInterested()) + abort(new IllegalStateException()); // Finish consuming the request // If we are still expecting diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/ProxyConnectionFactory.java b/jetty-server/src/main/java/org/eclipse/jetty/server/ProxyConnectionFactory.java index 3938c0f272a6..71cd03c274db 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/ProxyConnectionFactory.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/ProxyConnectionFactory.java @@ -27,7 +27,6 @@ import java.nio.channels.ReadPendingException; import java.nio.channels.WritePendingException; import java.nio.charset.StandardCharsets; -import java.util.function.Supplier; import org.eclipse.jetty.io.AbstractConnection; import org.eclipse.jetty.io.Connection; @@ -806,12 +805,6 @@ public void fillInterested(Callback callback) throws ReadPendingException _endp.fillInterested(callback); } - @Override - public Throwable cancelFillInterest(Supplier cancellation) - { - return _endp.cancelFillInterest(cancellation); - } - @Override public boolean flush(ByteBuffer... buffer) throws IOException { diff --git a/jetty-websocket/websocket-common/src/test/java/org/eclipse/jetty/websocket/common/io/MockEndPoint.java b/jetty-websocket/websocket-common/src/test/java/org/eclipse/jetty/websocket/common/io/MockEndPoint.java index 8a541ea10aee..b57c0f9bff9e 100644 --- a/jetty-websocket/websocket-common/src/test/java/org/eclipse/jetty/websocket/common/io/MockEndPoint.java +++ b/jetty-websocket/websocket-common/src/test/java/org/eclipse/jetty/websocket/common/io/MockEndPoint.java @@ -23,7 +23,6 @@ import java.nio.ByteBuffer; import java.nio.channels.ReadPendingException; import java.nio.channels.WritePendingException; -import java.util.function.Supplier; import org.eclipse.jetty.io.Connection; import org.eclipse.jetty.io.EndPoint; @@ -117,12 +116,6 @@ public void fillInterested(Callback callback) throws ReadPendingException throw new UnsupportedOperationException(NOT_SUPPORTED); } - @Override - public Throwable cancelFillInterest(Supplier cancellation) - { - throw new UnsupportedOperationException(NOT_SUPPORTED); - } - @Override public boolean tryFillInterested(Callback callback) { diff --git a/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/BlockedIOTest.java b/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/BlockedIOTest.java new file mode 100644 index 000000000000..4e4ac600bee7 --- /dev/null +++ b/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/BlockedIOTest.java @@ -0,0 +1,144 @@ +// +// ======================================================================== +// Copyright (c) 1995-2021 Mort Bay Consulting Pty Ltd and others. +// ------------------------------------------------------------------------ +// 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.http.client; + +import java.io.IOException; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; +import javax.servlet.ServletException; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import org.eclipse.jetty.client.util.DeferredContentProvider; +import org.eclipse.jetty.server.Request; +import org.eclipse.jetty.server.handler.AbstractHandler; +import org.eclipse.jetty.util.BufferUtil; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ArgumentsSource; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.instanceOf; +import static org.hamcrest.core.Is.is; +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class BlockedIOTest extends AbstractTest +{ + @Override + public void init(Transport transport) throws IOException + { + setScenario(new TransportScenario(transport)); + } + + @ParameterizedTest + @ArgumentsSource(TransportProvider.class) + public void testBlockingReadThenNormalComplete(Transport transport) throws Exception + { + CountDownLatch started = new CountDownLatch(1); + CountDownLatch stopped = new CountDownLatch(1); + AtomicReference readException = new AtomicReference<>(); + AtomicReference rereadException = new AtomicReference<>(); + + init(transport); + scenario.start(new AbstractHandler() + { + @Override + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + { + baseRequest.setHandled(true); + new Thread(() -> + { + try + { + int b = baseRequest.getHttpInput().read(); + if (b == '1') + { + started.countDown(); + if (baseRequest.getHttpInput().read() > Integer.MIN_VALUE) + throw new IllegalStateException(); + } + } + catch (Throwable ex1) + { + readException.set(ex1); + try + { + if (baseRequest.getHttpInput().read() > Integer.MIN_VALUE) + throw new IllegalStateException(); + } + catch (Throwable ex2) + { + rereadException.set(ex2); + } + finally + { + stopped.countDown(); + } + } + }).start(); + + try + { + // wait for thread to start and read first byte + started.await(10, TimeUnit.SECONDS); + // give it time to block on second byte + Thread.sleep(1000); + } + catch (Throwable e) + { + throw new ServletException(e); + } + + response.setStatus(200); + response.setContentType("text/plain"); + response.getOutputStream().print("OK\r\n"); + } + }); + + DeferredContentProvider contentProvider = new DeferredContentProvider(); + CountDownLatch ok = new CountDownLatch(2); + scenario.client.POST(scenario.newURI()) + .content(contentProvider) + .onResponseContent((response, content) -> + { + assertThat(BufferUtil.toString(content), containsString("OK")); + ok.countDown(); + }) + .onResponseSuccess(response -> + { + try + { + assertThat(response.getStatus(), is(200)); + stopped.await(10, TimeUnit.SECONDS); + ok.countDown(); + } + catch (Throwable t) + { + t.printStackTrace(); + } + }) + .send(null); + contentProvider.offer(BufferUtil.toBuffer("1")); + + assertTrue(ok.await(10, TimeUnit.SECONDS)); + assertThat(readException.get(), instanceOf(IOException.class)); + assertThat(rereadException.get(), instanceOf(IOException.class)); + } +} From 7235e492c8e370cbdfeee539f4d22f95b9f75505 Mon Sep 17 00:00:00 2001 From: gregw Date: Thu, 4 Feb 2021 09:58:24 +0100 Subject: [PATCH 10/33] Fix #5605 Unblock non container Threads Simplification. Always abort on any pending read or write in completion. --- .../eclipse/jetty/server/HttpConnection.java | 39 ++++++++++--------- .../org/eclipse/jetty/server/HttpOutput.java | 15 +++---- 2 files changed, 25 insertions(+), 29 deletions(-) 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 47caad8e43c0..be8b2c0992b9 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 @@ -376,39 +376,40 @@ private boolean parseRequestBuffer() @Override public void onCompleted() { + // If we are fill interested, then a read is pending and we must abort + if (isFillInterested()) + { + LOG.warn("Pending read in onCompleted {} {}", this, getEndPoint()); + abort(new IllegalStateException()); + } + // Handle connection upgrades if (_channel.getResponse().getStatus() == HttpStatus.SWITCHING_PROTOCOLS_101) { Connection connection = (Connection)_channel.getRequest().getAttribute(UPGRADE_CONNECTION_ATTRIBUTE); if (connection != null) { - if (isFillInterested()) - abort(new IllegalStateException()); + if (LOG.isDebugEnabled()) + LOG.debug("Upgrade from {} to {}", this, connection); + _channel.getState().upgrade(); + getEndPoint().upgrade(connection); + _channel.recycle(); + _parser.reset(); + _generator.reset(); + if (_contentBufferReferences.get() == 0) + releaseRequestBuffer(); else { - if (LOG.isDebugEnabled()) - LOG.debug("Upgrade from {} to {}", this, connection); - _channel.getState().upgrade(); - getEndPoint().upgrade(connection); - _channel.recycle(); - _parser.reset(); - _generator.reset(); - if (_contentBufferReferences.get() == 0) - releaseRequestBuffer(); - else - { - LOG.warn("{} lingering content references?!?!", this); - _requestBuffer = null; // Not returned to pool! - _contentBufferReferences.set(0); - } + LOG.warn("{} lingering content references?!?!", this); + _requestBuffer = null; // Not returned to pool! + _contentBufferReferences.set(0); } return; } } + // Drive to EOF, EarlyEOF or Error boolean complete = _input.consumeAll(); - if (isFillInterested()) - abort(new IllegalStateException()); // Finish consuming the request // If we are still expecting diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java index 248b85caccd0..2afa950d18c0 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java @@ -450,20 +450,15 @@ public void complete(Callback callback) break; case BLOCKED: - // An operation is in progress, so we soft close now - _softClose = true; - // then cancel the operation - CancellationException cancelled = new CancellationException(); - if (_writeBlocker.fail(cancelled)) - _channel.abort(cancelled); - // then trigger a close from onWriteComplete - _state = State.CLOSE; - break; - case UNREADY: case PENDING: + LOG.warn("Pending write onComplated {} {}", this, _channel); // An operation is in progress, so we soft close now _softClose = true; + // then cancel the operation and abort the channel + CancellationException cancelled = new CancellationException(); + _writeBlocker.fail(cancelled); + _channel.abort(cancelled); // then trigger a close from onWriteComplete _state = State.CLOSE; break; From 39f6f87ca7c5b01dc438a9a3c8425650c199b9dc Mon Sep 17 00:00:00 2001 From: gregw Date: Thu, 4 Feb 2021 10:26:11 +0100 Subject: [PATCH 11/33] Fix #5605 Unblock non container Threads Simplification. Always abort on any pending read or write in completion. --- .../java/org/eclipse/jetty/server/HttpOutput.java | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java index 2afa950d18c0..2b424ed40fb2 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java @@ -455,12 +455,19 @@ public void complete(Callback callback) LOG.warn("Pending write onComplated {} {}", this, _channel); // An operation is in progress, so we soft close now _softClose = true; - // then cancel the operation and abort the channel - CancellationException cancelled = new CancellationException(); - _writeBlocker.fail(cancelled); - _channel.abort(cancelled); // then trigger a close from onWriteComplete _state = State.CLOSE; + + // But if we are blocked or there is more content to come, we must abort + // Note that this allows a pending async write to complete only if it is the last write + if (_apiState == ApiState.BLOCKED || !_channel.getResponse().isContentComplete(_written)) + { + CancellationException cancelled = new CancellationException(); + _writeBlocker.fail(cancelled); + _channel.abort(cancelled); + _state = State.CLOSED; + } + break; } break; From ed534b84efa4f04dce0b047965a339ff797ffa20 Mon Sep 17 00:00:00 2001 From: gregw Date: Fri, 5 Feb 2021 15:36:34 +0100 Subject: [PATCH 12/33] Fix #5937 updates from review. --- .../main/java/org/eclipse/jetty/server/HttpInput.java | 9 --------- .../main/java/org/eclipse/jetty/server/HttpOutput.java | 2 +- .../src/main/java/org/eclipse/jetty/server/Request.java | 2 +- 3 files changed, 2 insertions(+), 11 deletions(-) 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 724132c3e303..91efb63e07b0 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 @@ -745,15 +745,6 @@ public boolean isError() } } - public Throwable getError() - { - synchronized (_inputQ) - { - Throwable error = _state instanceof ErrorState ? ((ErrorState)_state)._error : null; - return error == null ? new IOException() : error; - } - } - public boolean isAsync() { synchronized (_inputQ) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java index 2b424ed40fb2..9340ba11c117 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java @@ -452,7 +452,7 @@ public void complete(Callback callback) case BLOCKED: case UNREADY: case PENDING: - LOG.warn("Pending write onComplated {} {}", this, _channel); + LOG.warn("Pending write in complete {} {}", this, _channel); // An operation is in progress, so we soft close now _softClose = true; // then trigger a close from onWriteComplete diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java index 712745847f8e..634b37339991 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java @@ -1884,7 +1884,7 @@ protected void recycle() getHttpChannelState().recycle(); _requestAttributeListeners.clear(); - // Defer _input.recycle() until setMetaData on next request, so that late readers will fail + // Defer _input.recycle() until setMetaData on next request, TODO replace with recycle and reopen in 10 _metaData = null; _originalURI = null; _contextPath = null; From d297e9c4731a66ca198094e0fe3f2628c41c6d9a Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Mon, 8 Feb 2021 12:07:37 +0100 Subject: [PATCH 13/33] unblock readers after consumeAll Signed-off-by: Ludovic Orban --- .../jetty/server/AsyncContentProducer.java | 11 ++++++++++ .../jetty/server/BlockingContentProducer.java | 4 +++- .../org/eclipse/jetty/server/HttpInput.java | 1 - .../eclipse/jetty/server/BlockingTest.java | 21 +++++++------------ .../jetty/http/client/BlockedIOTest.java | 21 +++++++------------ 5 files changed, 30 insertions(+), 28 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java b/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java index e2864950bfb6..36478afaf54b 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java @@ -177,6 +177,10 @@ private void failCurrentContent(Throwable x) _rawContent.failed(x); _rawContent = null; } + + HttpInput.ErrorContent errorContent = new HttpInput.ErrorContent(x); + _transformedContent = errorContent; + _rawContent = errorContent; } @Override @@ -274,6 +278,13 @@ private HttpInput.Content nextTransformedContent() { // TODO does EOF need to be passed to the interceptors? + // In case the _rawContent was set by consumeAll(), check the httpChannel + // to see if it has a more precise error. Otherwise, the exact same + // special content will be returned by the httpChannel. + HttpInput.Content refreshedRawContent = produceRawContent(); + if (refreshedRawContent != null) + _rawContent = refreshedRawContent; + _error = _rawContent.getError() != null; if (LOG.isDebugEnabled()) LOG.debug("raw content is special (with error = {}), returning it {}", _error, this); diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/BlockingContentProducer.java b/jetty-server/src/main/java/org/eclipse/jetty/server/BlockingContentProducer.java index 186eb48dc8a1..c25748edbb66 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/BlockingContentProducer.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/BlockingContentProducer.java @@ -76,7 +76,9 @@ public long getRawContentArrived() @Override public boolean consumeAll(Throwable x) { - return _asyncContentProducer.consumeAll(x); + boolean eof = _asyncContentProducer.consumeAll(x); + _semaphore.release(); + return eof; } @Override 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 ff2edc7e06ec..a41ad47fe1f7 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 @@ -135,7 +135,6 @@ public boolean consumeAll() if (isFinished()) return !isError(); - //TODO move to early EOF and notify blocking reader return false; } diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingTest.java index 9a2f20b38287..f1f45c90aa78 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingTest.java @@ -1,19 +1,14 @@ // -// ======================================================================== -// Copyright (c) 1995-2021 Mort Bay Consulting Pty Ltd and others. -// ------------------------------------------------------------------------ -// 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. +// ======================================================================== +// Copyright (c) 1995-2021 Mort Bay Consulting Pty Ltd and others. // -// The Eclipse Public License is available at -// http://www.eclipse.org/legal/epl-v10.html +// This program and the accompanying materials are made available under the +// terms of the Eclipse Public License v. 2.0 which is available at +// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0 +// which is available at https://www.apache.org/licenses/LICENSE-2.0. // -// 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. -// ======================================================================== +// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 +// ======================================================================== // package org.eclipse.jetty.server; diff --git a/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/BlockedIOTest.java b/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/BlockedIOTest.java index 4e4ac600bee7..74b6bf408a0b 100644 --- a/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/BlockedIOTest.java +++ b/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/BlockedIOTest.java @@ -1,19 +1,14 @@ // -// ======================================================================== -// Copyright (c) 1995-2021 Mort Bay Consulting Pty Ltd and others. -// ------------------------------------------------------------------------ -// 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. +// ======================================================================== +// Copyright (c) 1995-2021 Mort Bay Consulting Pty Ltd and others. // -// The Eclipse Public License is available at -// http://www.eclipse.org/legal/epl-v10.html +// This program and the accompanying materials are made available under the +// terms of the Eclipse Public License v. 2.0 which is available at +// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0 +// which is available at https://www.apache.org/licenses/LICENSE-2.0. // -// 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. -// ======================================================================== +// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 +// ======================================================================== // package org.eclipse.jetty.http.client; From 50a379d5cf721a99c572b784eb8af02e69f28e50 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Tue, 9 Feb 2021 09:27:26 +0100 Subject: [PATCH 14/33] align HttpInput and HttpOutput on recycle and reopen Signed-off-by: Ludovic Orban --- .../src/main/java/org/eclipse/jetty/server/HttpInput.java | 6 ++++++ .../src/main/java/org/eclipse/jetty/server/Request.java | 4 ++-- 2 files changed, 8 insertions(+), 2 deletions(-) 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 a41ad47fe1f7..e5a0996e75a6 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 @@ -55,6 +55,12 @@ public void recycle() { if (LOG.isDebugEnabled()) LOG.debug("recycle {}", this); + } + + public void reopen() + { + if (LOG.isDebugEnabled()) + LOG.debug("reopen {}", this); _blockingContentProducer.recycle(); _contentProducer = _blockingContentProducer; _consumedEof = false; diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java index 28e417143525..61b635b0af50 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java @@ -1682,7 +1682,7 @@ public void setMetaData(MetaData.Request request) { if (_metaData == null && _input != null && _channel != null) { - _input.recycle(); + _input.reopen(); _channel.getResponse().getHttpOutput().reopen(); } _metaData = request; @@ -1776,7 +1776,7 @@ protected void recycle() getHttpChannelState().recycle(); _requestAttributeListeners.clear(); - // Defer _input.recycle() until setMetaData on next request, TODO replace with recycle and reopen in 10 + _input.recycle(); _metaData = null; _httpFields = null; _trailers = null; From f81ff1707e222660ecad8f514a013f431fa01af7 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Tue, 9 Feb 2021 15:26:10 +0100 Subject: [PATCH 15/33] move fill interested + pending read check out of upgrade Signed-off-by: Ludovic Orban --- .../org/eclipse/jetty/server/HttpConnection.java | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) 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 3fb493b1fad2..37d5224f640d 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 @@ -385,13 +385,6 @@ private boolean parseRequestBuffer() private boolean upgrade() { - // If we are fill interested, then a read is pending and we must abort - if (isFillInterested()) - { - LOG.warn("Pending read in onCompleted {} {}", this, getEndPoint()); - abort(new IllegalStateException()); - } - Connection connection = (Connection)_channel.getRequest().getAttribute(UPGRADE_CONNECTION_ATTRIBUTE); if (connection == null) return false; @@ -419,6 +412,13 @@ private boolean upgrade() @Override public void onCompleted() { + // If we are fill interested, then a read is pending and we must abort + if (isFillInterested()) + { + LOG.warn("Pending read in onCompleted {} {}", this, getEndPoint()); + abort(new IllegalStateException()); + } + // Handle connection upgrades. if (upgrade()) return; From a29a054fe135a8c516450551599082de48bfc92d Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Tue, 9 Feb 2021 17:02:54 +0100 Subject: [PATCH 16/33] fix leak Signed-off-by: Ludovic Orban --- .../main/java/org/eclipse/jetty/server/HttpConnection.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) 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 37d5224f640d..d77d48c787eb 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 @@ -319,7 +319,10 @@ void parseAndFillForContent() while (_parser.inContentState()) { boolean handled = parseRequestBuffer(); - if (handled || filled <= 0) + // Re-check the parser state after parsing to avoid filling, + // otherwise fillRequestBuffer() would acquire a ByteBuffer + // that may be leaked. + if (handled || filled <= 0 || !_parser.inContentState()) break; filled = fillRequestBuffer(); } From ce29e7cf1c6cc149caf4db09509418982cdaca1c Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Tue, 9 Feb 2021 17:12:17 +0100 Subject: [PATCH 17/33] nits Signed-off-by: Ludovic Orban --- .../main/java/org/eclipse/jetty/server/HttpConnection.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) 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 d77d48c787eb..59af89d671c9 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 @@ -309,7 +309,8 @@ else if (filled < 0) } /** - * Parse and fill data, looking for content + * Parse and fill data, looking for content. + * We do parse first, and only fill if we're out of bytes to avoid unnecessary system calls. */ void parseAndFillForContent() { @@ -318,11 +319,11 @@ void parseAndFillForContent() int filled = Integer.MAX_VALUE; while (_parser.inContentState()) { - boolean handled = parseRequestBuffer(); + boolean handle = parseRequestBuffer(); // Re-check the parser state after parsing to avoid filling, // otherwise fillRequestBuffer() would acquire a ByteBuffer // that may be leaked. - if (handled || filled <= 0 || !_parser.inContentState()) + if (handle || filled <= 0 || !_parser.inContentState()) break; filled = fillRequestBuffer(); } From 12734b149666d5ae029c60bcf5d8bc45950afa3d Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Tue, 9 Feb 2021 17:19:50 +0100 Subject: [PATCH 18/33] committee loop rewrite Signed-off-by: Ludovic Orban --- .../java/org/eclipse/jetty/server/HttpConnection.java | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) 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 59af89d671c9..36bc9b42697e 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 @@ -316,16 +316,17 @@ void parseAndFillForContent() { // When fillRequestBuffer() is called, it must always be followed by a parseRequestBuffer() call otherwise this method // doesn't trigger EOF/earlyEOF which breaks AsyncRequestReadTest.testPartialReadThenShutdown() - int filled = Integer.MAX_VALUE; + + // This loop was designed by a committee and voted by a majority. while (_parser.inContentState()) { - boolean handle = parseRequestBuffer(); + if (parseRequestBuffer()) + break; // Re-check the parser state after parsing to avoid filling, // otherwise fillRequestBuffer() would acquire a ByteBuffer // that may be leaked. - if (handle || filled <= 0 || !_parser.inContentState()) + if (_parser.inContentState() && fillRequestBuffer() <= 0) break; - filled = fillRequestBuffer(); } } From 3c4713db69b9e35b629031d855c4f6380a3dddd8 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Tue, 9 Feb 2021 17:47:11 +0100 Subject: [PATCH 19/33] nit Signed-off-by: Ludovic Orban --- .../src/main/java/org/eclipse/jetty/server/HttpConnection.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 36bc9b42697e..5891c730ee75 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 @@ -315,7 +315,7 @@ else if (filled < 0) void parseAndFillForContent() { // When fillRequestBuffer() is called, it must always be followed by a parseRequestBuffer() call otherwise this method - // doesn't trigger EOF/earlyEOF which breaks AsyncRequestReadTest.testPartialReadThenShutdown() + // doesn't trigger EOF/earlyEOF which breaks AsyncRequestReadTest.testPartialReadThenShutdown(). // This loop was designed by a committee and voted by a majority. while (_parser.inContentState()) From 9f2a4f5ad53c6bb3fc74cba45acf8411ceef4512 Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 10 Feb 2021 15:35:48 +0100 Subject: [PATCH 20/33] Fix #5605 write side refactored the complete method to consider unrecoverable API states no matter what the httpout state actually is. This avoid duplication of OPEN, CLOSING, CLOSED etc. handling. --- .../org/eclipse/jetty/server/HttpOutput.java | 120 +++++++++++------- .../jetty/http/client/BlockedIOTest.java | 3 +- 2 files changed, 73 insertions(+), 50 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java index 9340ba11c117..970eaf9a5b40 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java @@ -412,65 +412,87 @@ public void complete(Callback callback) ByteBuffer content = null; synchronized (_channelState) { - switch (_state) + // First check the API state for any unrecoverable situations + switch (_apiState) { - case CLOSED: - succeeded = true; + case UNREADY: // isReady() has returned false so a call to onWritePossible may happen at any time + error = new CancellationException("Completed whilst write unready"); break; - case CLOSE: - case CLOSING: - _closedCallback = Callback.combine(_closedCallback, callback); + case PENDING: // an async write is pending and may complete at any time + // If this is not the last write, then we must abort + if (!_channel.getResponse().isContentComplete(_written)) + error = new CancellationException("Completed whilst write pending"); break; - case OPEN: - if (_onError != null) - { - error = _onError; - break; - } - - _closedCallback = Callback.combine(_closedCallback, callback); - - switch (_apiState) - { - case BLOCKING: - // Output is idle blocking state, but we still do an async close - _apiState = ApiState.BLOCKED; - _state = State.CLOSING; - content = BufferUtil.hasContent(_aggregate) ? _aggregate : BufferUtil.EMPTY_BUFFER; - break; + case BLOCKED: // another thread is blocked in a write or a close + error = new CancellationException("Completed whilst write blocked"); + break; - case ASYNC: - case READY: - // Output is idle in async state, so we can do an async close - _apiState = ApiState.PENDING; - _state = State.CLOSING; - content = BufferUtil.hasContent(_aggregate) ? _aggregate : BufferUtil.EMPTY_BUFFER; - break; + default: + break; + } - case BLOCKED: - case UNREADY: - case PENDING: - LOG.warn("Pending write in complete {} {}", this, _channel); - // An operation is in progress, so we soft close now - _softClose = true; - // then trigger a close from onWriteComplete - _state = State.CLOSE; + // If we can't complete due to the API state, then abort + if (error != null) + { + _writeBlocker.fail(error); + _channel.abort(error); + _state = State.CLOSED; + } + else + { + // Otherwise check the output state to determine how to complete + switch (_state) + { + case CLOSED: + succeeded = true; + break; - // But if we are blocked or there is more content to come, we must abort - // Note that this allows a pending async write to complete only if it is the last write - if (_apiState == ApiState.BLOCKED || !_channel.getResponse().isContentComplete(_written)) - { - CancellationException cancelled = new CancellationException(); - _writeBlocker.fail(cancelled); - _channel.abort(cancelled); - _state = State.CLOSED; - } + case CLOSE: + case CLOSING: + _closedCallback = Callback.combine(_closedCallback, callback); + break; + case OPEN: + if (_onError != null) + { + error = _onError; break; - } - break; + } + + _closedCallback = Callback.combine(_closedCallback, callback); + + switch (_apiState) + { + case BLOCKING: + // Output is idle blocking state, but we still do an async close + _apiState = ApiState.BLOCKED; + _state = State.CLOSING; + content = BufferUtil.hasContent(_aggregate) ? _aggregate : BufferUtil.EMPTY_BUFFER; + break; + + case ASYNC: + case READY: + // Output is idle in async state, so we can do an async close + _apiState = ApiState.PENDING; + _state = State.CLOSING; + content = BufferUtil.hasContent(_aggregate) ? _aggregate : BufferUtil.EMPTY_BUFFER; + break; + + case UNREADY: + case PENDING: + // An operation is in progress, so we soft close now + _softClose = true; + // then trigger a close from onWriteComplete + _state = State.CLOSE; + break; + + default: + throw new IllegalStateException(); + } + break; + } } } diff --git a/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/BlockedIOTest.java b/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/BlockedIOTest.java index 4e4ac600bee7..ca14214e9b4d 100644 --- a/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/BlockedIOTest.java +++ b/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/BlockedIOTest.java @@ -114,7 +114,8 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques DeferredContentProvider contentProvider = new DeferredContentProvider(); CountDownLatch ok = new CountDownLatch(2); - scenario.client.POST(scenario.newURI()) + scenario.client.newRequest(scenario.newURI()) + .method("POST") .content(contentProvider) .onResponseContent((response, content) -> { From 3c46c53d91dfbb101843faa191b1a657c3f14a7f Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Wed, 10 Feb 2021 15:49:55 +0100 Subject: [PATCH 21/33] fix the committee's loop Signed-off-by: Ludovic Orban --- .../src/main/java/org/eclipse/jetty/server/HttpConnection.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 5891c730ee75..dea38f8a3c90 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 @@ -325,7 +325,7 @@ void parseAndFillForContent() // Re-check the parser state after parsing to avoid filling, // otherwise fillRequestBuffer() would acquire a ByteBuffer // that may be leaked. - if (_parser.inContentState() && fillRequestBuffer() <= 0) + if (_parser.inContentState() || fillRequestBuffer() <= 0) break; } } From 769687f773808615dced710965e5ba5b245bf50a Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 10 Feb 2021 16:38:12 +0100 Subject: [PATCH 22/33] update from the feedback on the feedback of the feedback from the review. fix javadoc --- .../src/main/java/org/eclipse/jetty/server/HttpOutput.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java index 970eaf9a5b40..ee6cc0e908df 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java @@ -70,7 +70,7 @@ public class HttpOutput extends ServletOutputStream implements Runnable enum State { OPEN, // Open - CLOSE, // Close needed from onWriteCompletion + CLOSE, // Close needed from onWriteComplete CLOSING, // Close in progress after close API called CLOSED // Closed } @@ -309,7 +309,7 @@ else if (_state == State.CLOSE) { // Somebody called close or complete while we were writing. // We can now send a (probably empty) last buffer and then when it completes - // onWriteCompletion will be called again to actually execute the _completeCallback + // onWriteComplete will be called again to actually execute the _completeCallback _state = State.CLOSING; closeContent = BufferUtil.hasContent(_aggregate) ? _aggregate : BufferUtil.EMPTY_BUFFER; } From f8bf885686b4276f38506361072448068c8e2ef2 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Wed, 10 Feb 2021 18:02:37 +0100 Subject: [PATCH 23/33] restore the committee's loop Signed-off-by: Ludovic Orban --- .../src/main/java/org/eclipse/jetty/server/HttpConnection.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 dea38f8a3c90..5891c730ee75 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 @@ -325,7 +325,7 @@ void parseAndFillForContent() // Re-check the parser state after parsing to avoid filling, // otherwise fillRequestBuffer() would acquire a ByteBuffer // that may be leaked. - if (_parser.inContentState() || fillRequestBuffer() <= 0) + if (_parser.inContentState() && fillRequestBuffer() <= 0) break; } } From 6d9d5484a7330a650179f4ebd114451a33a72aea Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Wed, 10 Feb 2021 18:03:10 +0100 Subject: [PATCH 24/33] lock all HttpInput methods to prevents concurrent threads to work on the same ByteBuffer Signed-off-by: Ludovic Orban --- .../org/eclipse/jetty/server/HttpInput.java | 319 +++++++++++------- 1 file changed, 191 insertions(+), 128 deletions(-) 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 e5a0996e75a6..dcdd99b1e4b1 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 @@ -23,6 +23,7 @@ import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.component.Destroyable; +import org.eclipse.jetty.util.thread.AutoLock; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -42,13 +43,17 @@ public class HttpInput extends ServletInputStream implements Runnable private boolean _consumedEof; private ReadListener _readListener; private long _contentConsumed; + private final AutoLock _lock = new AutoLock(); public HttpInput(HttpChannelState state) { - _channelState = state; - _asyncContentProducer = new AsyncContentProducer(state.getHttpChannel()); - _blockingContentProducer = new BlockingContentProducer(_asyncContentProducer); - _contentProducer = _blockingContentProducer; + try (AutoLock lock = _lock.lock()) + { + _channelState = state; + _asyncContentProducer = new AsyncContentProducer(state.getHttpChannel()); + _blockingContentProducer = new BlockingContentProducer(_asyncContentProducer); + _contentProducer = _blockingContentProducer; + } } public void recycle() @@ -59,13 +64,16 @@ public void recycle() public void reopen() { - if (LOG.isDebugEnabled()) - LOG.debug("reopen {}", this); - _blockingContentProducer.recycle(); - _contentProducer = _blockingContentProducer; - _consumedEof = false; - _readListener = null; - _contentConsumed = 0; + try (AutoLock lock = _lock.lock()) + { + if (LOG.isDebugEnabled()) + LOG.debug("reopen {}", this); + _blockingContentProducer.recycle(); + _contentProducer = _blockingContentProducer; + _consumedEof = false; + _readListener = null; + _contentConsumed = 0; + } } /** @@ -73,7 +81,10 @@ public void reopen() */ public Interceptor getInterceptor() { - return _contentProducer.getInterceptor(); + try (AutoLock lock = _lock.lock()) + { + return _contentProducer.getInterceptor(); + } } /** @@ -83,9 +94,12 @@ public Interceptor getInterceptor() */ public void setInterceptor(Interceptor interceptor) { - if (LOG.isDebugEnabled()) - LOG.debug("setting interceptor to {} on {}", interceptor, this); - _contentProducer.setInterceptor(interceptor); + try (AutoLock lock = _lock.lock()) + { + if (LOG.isDebugEnabled()) + LOG.debug("setting interceptor to {} on {}", interceptor, this); + _contentProducer.setInterceptor(interceptor); + } } /** @@ -96,23 +110,26 @@ public void setInterceptor(Interceptor interceptor) */ public void addInterceptor(Interceptor interceptor) { - Interceptor currentInterceptor = _contentProducer.getInterceptor(); - if (currentInterceptor == null) - { - if (LOG.isDebugEnabled()) - LOG.debug("adding single interceptor: {} on {}", interceptor, this); - _contentProducer.setInterceptor(interceptor); - } - else + try (AutoLock lock = _lock.lock()) { - ChainedInterceptor chainedInterceptor = new ChainedInterceptor(currentInterceptor, interceptor); - if (LOG.isDebugEnabled()) - LOG.debug("adding chained interceptor: {} on {}", chainedInterceptor, this); - _contentProducer.setInterceptor(chainedInterceptor); + Interceptor currentInterceptor = _contentProducer.getInterceptor(); + if (currentInterceptor == null) + { + if (LOG.isDebugEnabled()) + LOG.debug("adding single interceptor: {} on {}", interceptor, this); + _contentProducer.setInterceptor(interceptor); + } + else + { + ChainedInterceptor chainedInterceptor = new ChainedInterceptor(currentInterceptor, interceptor); + if (LOG.isDebugEnabled()) + LOG.debug("adding chained interceptor: {} on {}", chainedInterceptor, this); + _contentProducer.setInterceptor(chainedInterceptor); + } } } - public int get(Content content, byte[] bytes, int offset, int length) + private int get(Content content, byte[] bytes, int offset, int length) { int consumed = content.get(bytes, offset, length); _contentConsumed += consumed; @@ -121,42 +138,57 @@ public int get(Content content, byte[] bytes, int offset, int length) public long getContentConsumed() { - return _contentConsumed; + try (AutoLock lock = _lock.lock()) + { + return _contentConsumed; + } } public long getContentReceived() { - return _contentProducer.getRawContentArrived(); + try (AutoLock lock = _lock.lock()) + { + return _contentProducer.getRawContentArrived(); + } } public boolean consumeAll() { - IOException failure = new IOException("Unconsumed content"); - if (LOG.isDebugEnabled()) - LOG.debug("consumeAll {}", this, failure); - boolean atEof = _contentProducer.consumeAll(failure); - if (atEof) - _consumedEof = true; + try (AutoLock lock = _lock.lock()) + { + IOException failure = new IOException("Unconsumed content"); + if (LOG.isDebugEnabled()) + LOG.debug("consumeAll {}", this, failure); + boolean atEof = _contentProducer.consumeAll(failure); + if (atEof) + _consumedEof = true; - if (isFinished()) - return !isError(); + if (isFinished()) + return !isError(); - return false; + return false; + } } public boolean isError() { - boolean error = _contentProducer.isError(); - if (LOG.isDebugEnabled()) - LOG.debug("isError={} {}", error, this); - return error; + try (AutoLock lock = _lock.lock()) + { + boolean error = _contentProducer.isError(); + if (LOG.isDebugEnabled()) + LOG.debug("isError={} {}", error, this); + return error; + } } public boolean isAsync() { - if (LOG.isDebugEnabled()) - LOG.debug("isAsync read listener {} {}", _readListener, this); - return _readListener != null; + try (AutoLock lock = _lock.lock()) + { + if (LOG.isDebugEnabled()) + LOG.debug("isAsync read listener {} {}", _readListener, this); + return _readListener != null; + } } /* ServletInputStream */ @@ -164,94 +196,112 @@ public boolean isAsync() @Override public boolean isFinished() { - boolean finished = _consumedEof; - if (LOG.isDebugEnabled()) - LOG.debug("isFinished={} {}", finished, this); - return finished; + try (AutoLock lock = _lock.lock()) + { + boolean finished = _consumedEof; + if (LOG.isDebugEnabled()) + LOG.debug("isFinished={} {}", finished, this); + return finished; + } } @Override public boolean isReady() { - boolean ready = _contentProducer.isReady(); - if (LOG.isDebugEnabled()) - LOG.debug("isReady={} {}", ready, this); - return ready; + try (AutoLock lock = _lock.lock()) + { + boolean ready = _contentProducer.isReady(); + if (LOG.isDebugEnabled()) + LOG.debug("isReady={} {}", ready, this); + return ready; + } } @Override public void setReadListener(ReadListener readListener) { - if (LOG.isDebugEnabled()) - LOG.debug("setting read listener to {} {}", readListener, this); - if (_readListener != null) - throw new IllegalStateException("ReadListener already set"); - _readListener = Objects.requireNonNull(readListener); - //illegal if async not started - if (!_channelState.isAsyncStarted()) - throw new IllegalStateException("Async not started"); - - _contentProducer = _asyncContentProducer; - // trigger content production - if (isReady() && _channelState.onReadEof()) // onReadEof b/c we want to transition from WAITING to WOKEN - scheduleReadListenerNotification(); // this is needed by AsyncServletIOTest.testStolenAsyncRead + try (AutoLock lock = _lock.lock()) + { + if (LOG.isDebugEnabled()) + LOG.debug("setting read listener to {} {}", readListener, this); + if (_readListener != null) + throw new IllegalStateException("ReadListener already set"); + _readListener = Objects.requireNonNull(readListener); + //illegal if async not started + if (!_channelState.isAsyncStarted()) + throw new IllegalStateException("Async not started"); + + _contentProducer = _asyncContentProducer; + // trigger content production + if (isReady() && _channelState.onReadEof()) // onReadEof b/c we want to transition from WAITING to WOKEN + scheduleReadListenerNotification(); // this is needed by AsyncServletIOTest.testStolenAsyncRead + } } public boolean onContentProducible() { - return _contentProducer.onContentProducible(); + try (AutoLock lock = _lock.lock()) + { + return _contentProducer.onContentProducible(); + } } @Override public int read() throws IOException { - int read = read(_oneByteBuffer, 0, 1); - if (read == 0) - throw new IOException("unready read=0"); - return read < 0 ? -1 : _oneByteBuffer[0] & 0xFF; + try (AutoLock lock = _lock.lock()) + { + int read = read(_oneByteBuffer, 0, 1); + if (read == 0) + throw new IOException("unready read=0"); + return read < 0 ? -1 : _oneByteBuffer[0] & 0xFF; + } } @Override public int read(byte[] b, int off, int len) throws IOException { - // Calculate minimum request rate for DoS protection - _contentProducer.checkMinDataRate(); - - Content content = _contentProducer.nextContent(); - if (content == null) - throw new IllegalStateException("read on unready input"); - if (!content.isSpecial()) + try (AutoLock lock = _lock.lock()) { - int read = get(content, b, off, len); - if (LOG.isDebugEnabled()) - LOG.debug("read produced {} byte(s) {}", read, this); - if (content.isEmpty()) - _contentProducer.reclaim(content); - return read; - } + // Calculate minimum request rate for DoS protection + _contentProducer.checkMinDataRate(); - Throwable error = content.getError(); - if (LOG.isDebugEnabled()) - LOG.debug("read error={} {}", error, this); - if (error != null) - { - if (error instanceof IOException) - throw (IOException)error; - throw new IOException(error); - } + Content content = _contentProducer.nextContent(); + if (content == null) + throw new IllegalStateException("read on unready input"); + if (!content.isSpecial()) + { + int read = get(content, b, off, len); + if (LOG.isDebugEnabled()) + LOG.debug("read produced {} byte(s) {}", read, this); + if (content.isEmpty()) + _contentProducer.reclaim(content); + return read; + } - if (content.isEof()) - { + Throwable error = content.getError(); if (LOG.isDebugEnabled()) - LOG.debug("read at EOF, setting consumed EOF to true {}", this); - _consumedEof = true; - // If EOF do we need to wake for allDataRead callback? - if (onContentProducible()) - scheduleReadListenerNotification(); - return -1; - } + LOG.debug("read error={} {}", error, this); + if (error != null) + { + if (error instanceof IOException) + throw (IOException)error; + throw new IOException(error); + } + + if (content.isEof()) + { + if (LOG.isDebugEnabled()) + LOG.debug("read at EOF, setting consumed EOF to true {}", this); + _consumedEof = true; + // If EOF do we need to wake for allDataRead callback? + if (onContentProducible()) + scheduleReadListenerNotification(); + return -1; + } - throw new AssertionError("no data, no error and not EOF"); + throw new AssertionError("no data, no error and not EOF"); + } } private void scheduleReadListenerNotification() @@ -267,21 +317,27 @@ private void scheduleReadListenerNotification() */ public boolean hasContent() { - // Do not call _contentProducer.available() as it calls HttpChannel.produceContent() - // which is forbidden by this method's contract. - boolean hasContent = _contentProducer.hasContent(); - if (LOG.isDebugEnabled()) - LOG.debug("hasContent={} {}", hasContent, this); - return hasContent; + try (AutoLock lock = _lock.lock()) + { + // Do not call _contentProducer.available() as it calls HttpChannel.produceContent() + // which is forbidden by this method's contract. + boolean hasContent = _contentProducer.hasContent(); + if (LOG.isDebugEnabled()) + LOG.debug("hasContent={} {}", hasContent, this); + return hasContent; + } } @Override public int available() { - int available = _contentProducer.available(); - if (LOG.isDebugEnabled()) - LOG.debug("available={} {}", available, this); - return available; + try (AutoLock lock = _lock.lock()) + { + int available = _contentProducer.available(); + if (LOG.isDebugEnabled()) + LOG.debug("available={} {}", available, this); + return available; + } } /* Runnable */ @@ -293,19 +349,26 @@ public int available() @Override public void run() { - // Call isReady() to make sure that if not ready we register for fill interest. - if (!_contentProducer.isReady()) + Content content; + ReadListener readListener; + try (AutoLock lock = _lock.lock()) { + // Call isReady() to make sure that if not ready we register for fill interest. + if (!_contentProducer.isReady()) + { + if (LOG.isDebugEnabled()) + LOG.debug("running but not ready {}", this); + return; + } + content = _contentProducer.nextContent(); if (LOG.isDebugEnabled()) - LOG.debug("running but not ready {}", this); - return; + LOG.debug("running on content {} {}", content, this); + + readListener = this._readListener; } - Content content = _contentProducer.nextContent(); - if (LOG.isDebugEnabled()) - LOG.debug("running on content {} {}", content, this); // This check is needed when a request is started async but no read listener is registered. - if (_readListener == null) + if (readListener == null) { if (LOG.isDebugEnabled()) LOG.debug("running without a read listener {}", this); @@ -322,7 +385,7 @@ public void run() LOG.debug("running error={} {}", error, this); // TODO is this necessary to add here? _channelState.getHttpChannel().getResponse().getHttpFields().add(HttpConnection.CONNECTION_CLOSE); - _readListener.onError(error); + readListener.onError(error); } else if (content.isEof()) { @@ -330,13 +393,13 @@ else if (content.isEof()) { if (LOG.isDebugEnabled()) LOG.debug("running at EOF {}", this); - _readListener.onAllDataRead(); + readListener.onAllDataRead(); } catch (Throwable x) { if (LOG.isDebugEnabled()) LOG.debug("running failed onAllDataRead {}", this, x); - _readListener.onError(x); + readListener.onError(x); } } } @@ -346,13 +409,13 @@ else if (content.isEof()) LOG.debug("running has content {}", this); try { - _readListener.onDataAvailable(); + readListener.onDataAvailable(); } catch (Throwable x) { if (LOG.isDebugEnabled()) LOG.debug("running failed onDataAvailable {}", this, x); - _readListener.onError(x); + readListener.onError(x); } } } From 1494a053270446678bbe4ff58b18925230825056 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Thu, 11 Feb 2021 10:23:18 +0100 Subject: [PATCH 25/33] rework HttpInput locking Signed-off-by: Ludovic Orban --- .../org/eclipse/jetty/server/HttpInput.java | 141 ++++++++---------- 1 file changed, 60 insertions(+), 81 deletions(-) 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 dcdd99b1e4b1..8336b2443349 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 @@ -16,6 +16,7 @@ import java.io.IOException; import java.nio.ByteBuffer; import java.util.Objects; +import java.util.concurrent.atomic.LongAdder; import javax.servlet.ReadListener; import javax.servlet.ServletInputStream; @@ -38,22 +39,19 @@ public class HttpInput extends ServletInputStream implements Runnable private final byte[] _oneByteBuffer = new byte[1]; private final BlockingContentProducer _blockingContentProducer; private final AsyncContentProducer _asyncContentProducer; + private final AutoLock _contentProducerLock = new AutoLock(); private final HttpChannelState _channelState; - private ContentProducer _contentProducer; - private boolean _consumedEof; - private ReadListener _readListener; - private long _contentConsumed; - private final AutoLock _lock = new AutoLock(); + private final LongAdder _contentConsumed = new LongAdder(); + private volatile ContentProducer _contentProducer; + private volatile boolean _consumedEof; + private volatile ReadListener _readListener; public HttpInput(HttpChannelState state) { - try (AutoLock lock = _lock.lock()) - { - _channelState = state; - _asyncContentProducer = new AsyncContentProducer(state.getHttpChannel()); - _blockingContentProducer = new BlockingContentProducer(_asyncContentProducer); - _contentProducer = _blockingContentProducer; - } + _channelState = state; + _asyncContentProducer = new AsyncContentProducer(state.getHttpChannel()); + _blockingContentProducer = new BlockingContentProducer(_asyncContentProducer); + _contentProducer = _blockingContentProducer; } public void recycle() @@ -64,16 +62,13 @@ public void recycle() public void reopen() { - try (AutoLock lock = _lock.lock()) - { - if (LOG.isDebugEnabled()) - LOG.debug("reopen {}", this); - _blockingContentProducer.recycle(); - _contentProducer = _blockingContentProducer; - _consumedEof = false; - _readListener = null; - _contentConsumed = 0; - } + if (LOG.isDebugEnabled()) + LOG.debug("reopen {}", this); + _blockingContentProducer.recycle(); + _contentProducer = _blockingContentProducer; + _consumedEof = false; + _readListener = null; + _contentConsumed.reset(); } /** @@ -81,7 +76,7 @@ public void reopen() */ public Interceptor getInterceptor() { - try (AutoLock lock = _lock.lock()) + try (AutoLock lock = _contentProducerLock.lock()) { return _contentProducer.getInterceptor(); } @@ -94,7 +89,7 @@ public Interceptor getInterceptor() */ public void setInterceptor(Interceptor interceptor) { - try (AutoLock lock = _lock.lock()) + try (AutoLock lock = _contentProducerLock.lock()) { if (LOG.isDebugEnabled()) LOG.debug("setting interceptor to {} on {}", interceptor, this); @@ -110,7 +105,7 @@ public void setInterceptor(Interceptor interceptor) */ public void addInterceptor(Interceptor interceptor) { - try (AutoLock lock = _lock.lock()) + try (AutoLock lock = _contentProducerLock.lock()) { Interceptor currentInterceptor = _contentProducer.getInterceptor(); if (currentInterceptor == null) @@ -132,21 +127,18 @@ public void addInterceptor(Interceptor interceptor) private int get(Content content, byte[] bytes, int offset, int length) { int consumed = content.get(bytes, offset, length); - _contentConsumed += consumed; + _contentConsumed.add(consumed); return consumed; } public long getContentConsumed() { - try (AutoLock lock = _lock.lock()) - { - return _contentConsumed; - } + return _contentConsumed.sum(); } public long getContentReceived() { - try (AutoLock lock = _lock.lock()) + try (AutoLock lock = _contentProducerLock.lock()) { return _contentProducer.getRawContentArrived(); } @@ -154,7 +146,7 @@ public long getContentReceived() public boolean consumeAll() { - try (AutoLock lock = _lock.lock()) + try (AutoLock lock = _contentProducerLock.lock()) { IOException failure = new IOException("Unconsumed content"); if (LOG.isDebugEnabled()) @@ -172,7 +164,7 @@ public boolean consumeAll() public boolean isError() { - try (AutoLock lock = _lock.lock()) + try (AutoLock lock = _contentProducerLock.lock()) { boolean error = _contentProducer.isError(); if (LOG.isDebugEnabled()) @@ -183,12 +175,9 @@ public boolean isError() public boolean isAsync() { - try (AutoLock lock = _lock.lock()) - { - if (LOG.isDebugEnabled()) - LOG.debug("isAsync read listener {} {}", _readListener, this); - return _readListener != null; - } + if (LOG.isDebugEnabled()) + LOG.debug("isAsync read listener {} {}", _readListener, this); + return _readListener != null; } /* ServletInputStream */ @@ -196,19 +185,16 @@ public boolean isAsync() @Override public boolean isFinished() { - try (AutoLock lock = _lock.lock()) - { - boolean finished = _consumedEof; - if (LOG.isDebugEnabled()) - LOG.debug("isFinished={} {}", finished, this); - return finished; - } + boolean finished = _consumedEof; + if (LOG.isDebugEnabled()) + LOG.debug("isFinished={} {}", finished, this); + return finished; } @Override public boolean isReady() { - try (AutoLock lock = _lock.lock()) + try (AutoLock lock = _contentProducerLock.lock()) { boolean ready = _contentProducer.isReady(); if (LOG.isDebugEnabled()) @@ -220,36 +206,32 @@ public boolean isReady() @Override public void setReadListener(ReadListener readListener) { - try (AutoLock lock = _lock.lock()) - { - if (LOG.isDebugEnabled()) - LOG.debug("setting read listener to {} {}", readListener, this); - if (_readListener != null) - throw new IllegalStateException("ReadListener already set"); - _readListener = Objects.requireNonNull(readListener); - //illegal if async not started - if (!_channelState.isAsyncStarted()) - throw new IllegalStateException("Async not started"); - - _contentProducer = _asyncContentProducer; - // trigger content production - if (isReady() && _channelState.onReadEof()) // onReadEof b/c we want to transition from WAITING to WOKEN - scheduleReadListenerNotification(); // this is needed by AsyncServletIOTest.testStolenAsyncRead - } + if (LOG.isDebugEnabled()) + LOG.debug("setting read listener to {} {}", readListener, this); + if (_readListener != null) + throw new IllegalStateException("ReadListener already set"); + _readListener = Objects.requireNonNull(readListener); + //illegal if async not started + if (!_channelState.isAsyncStarted()) + throw new IllegalStateException("Async not started"); + + _contentProducer = _asyncContentProducer; + // trigger content production + if (isReady() && _channelState.onReadEof()) // onReadEof b/c we want to transition from WAITING to WOKEN + scheduleReadListenerNotification(); // this is needed by AsyncServletIOTest.testStolenAsyncRead } public boolean onContentProducible() { - try (AutoLock lock = _lock.lock()) - { - return _contentProducer.onContentProducible(); - } + // This is the only _contentProducer method call that must not + // happen under a lock as it is used to release blocking calls. + return _contentProducer.onContentProducible(); } @Override public int read() throws IOException { - try (AutoLock lock = _lock.lock()) + try (AutoLock lock = _contentProducerLock.lock()) { int read = read(_oneByteBuffer, 0, 1); if (read == 0) @@ -261,7 +243,7 @@ public int read() throws IOException @Override public int read(byte[] b, int off, int len) throws IOException { - try (AutoLock lock = _lock.lock()) + try (AutoLock lock = _contentProducerLock.lock()) { // Calculate minimum request rate for DoS protection _contentProducer.checkMinDataRate(); @@ -317,7 +299,7 @@ private void scheduleReadListenerNotification() */ public boolean hasContent() { - try (AutoLock lock = _lock.lock()) + try (AutoLock lock = _contentProducerLock.lock()) { // Do not call _contentProducer.available() as it calls HttpChannel.produceContent() // which is forbidden by this method's contract. @@ -331,7 +313,7 @@ public boolean hasContent() @Override public int available() { - try (AutoLock lock = _lock.lock()) + try (AutoLock lock = _contentProducerLock.lock()) { int available = _contentProducer.available(); if (LOG.isDebugEnabled()) @@ -350,8 +332,7 @@ public int available() public void run() { Content content; - ReadListener readListener; - try (AutoLock lock = _lock.lock()) + try (AutoLock lock = _contentProducerLock.lock()) { // Call isReady() to make sure that if not ready we register for fill interest. if (!_contentProducer.isReady()) @@ -363,12 +344,10 @@ public void run() content = _contentProducer.nextContent(); if (LOG.isDebugEnabled()) LOG.debug("running on content {} {}", content, this); - - readListener = this._readListener; } // This check is needed when a request is started async but no read listener is registered. - if (readListener == null) + if (_readListener == null) { if (LOG.isDebugEnabled()) LOG.debug("running without a read listener {}", this); @@ -385,7 +364,7 @@ public void run() LOG.debug("running error={} {}", error, this); // TODO is this necessary to add here? _channelState.getHttpChannel().getResponse().getHttpFields().add(HttpConnection.CONNECTION_CLOSE); - readListener.onError(error); + _readListener.onError(error); } else if (content.isEof()) { @@ -393,13 +372,13 @@ else if (content.isEof()) { if (LOG.isDebugEnabled()) LOG.debug("running at EOF {}", this); - readListener.onAllDataRead(); + _readListener.onAllDataRead(); } catch (Throwable x) { if (LOG.isDebugEnabled()) LOG.debug("running failed onAllDataRead {}", this, x); - readListener.onError(x); + _readListener.onError(x); } } } @@ -409,13 +388,13 @@ else if (content.isEof()) LOG.debug("running has content {}", this); try { - readListener.onDataAvailable(); + _readListener.onDataAvailable(); } catch (Throwable x) { if (LOG.isDebugEnabled()) LOG.debug("running failed onDataAvailable {}", this, x); - readListener.onError(x); + _readListener.onError(x); } } } From 14108c8e58214164cea5e3304106a3a00f9a5258 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Thu, 11 Feb 2021 10:24:48 +0100 Subject: [PATCH 26/33] set the read listener only after all checks are done Signed-off-by: Ludovic Orban --- .../src/main/java/org/eclipse/jetty/server/HttpInput.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 8336b2443349..0b879d5ae215 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 @@ -210,10 +210,10 @@ public void setReadListener(ReadListener readListener) LOG.debug("setting read listener to {} {}", readListener, this); if (_readListener != null) throw new IllegalStateException("ReadListener already set"); - _readListener = Objects.requireNonNull(readListener); //illegal if async not started if (!_channelState.isAsyncStarted()) throw new IllegalStateException("Async not started"); + _readListener = Objects.requireNonNull(readListener); _contentProducer = _asyncContentProducer; // trigger content production From e2c710e0868746673c21f6aace99637feb3605b1 Mon Sep 17 00:00:00 2001 From: gregw Date: Thu, 11 Feb 2021 15:01:00 +0100 Subject: [PATCH 27/33] updates from review --- .../src/main/java/org/eclipse/jetty/server/HttpConnection.java | 2 +- .../src/main/java/org/eclipse/jetty/server/HttpOutput.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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 be8b2c0992b9..0e31a13ea8dc 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 @@ -384,7 +384,7 @@ public void onCompleted() } // Handle connection upgrades - if (_channel.getResponse().getStatus() == HttpStatus.SWITCHING_PROTOCOLS_101) + else if (_channel.getResponse().getStatus() == HttpStatus.SWITCHING_PROTOCOLS_101) { Connection connection = (Connection)_channel.getRequest().getAttribute(UPGRADE_CONNECTION_ATTRIBUTE); if (connection != null) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java index ee6cc0e908df..675d1d1c622f 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java @@ -436,8 +436,8 @@ public void complete(Callback callback) // If we can't complete due to the API state, then abort if (error != null) { - _writeBlocker.fail(error); _channel.abort(error); + _writeBlocker.fail(error); _state = State.CLOSED; } else From 59b397b1a07ac0b71ef7f4c3cfc354d05285c057 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Thu, 11 Feb 2021 15:33:54 +0100 Subject: [PATCH 28/33] fix deadlock between HttpInput's lock and BlockingContentProducer's semaphore Signed-off-by: Ludovic Orban --- .../jetty/server/AsyncContentProducer.java | 8 ++++-- .../jetty/server/BlockingContentProducer.java | 25 ++++++++++++++++--- .../eclipse/jetty/server/ContentProducer.java | 8 ++++-- .../org/eclipse/jetty/server/HttpInput.java | 4 +-- .../server/AsyncContentProducerTest.java | 17 ++++++++----- .../server/BlockingContentProducerTest.java | 8 +++++- 6 files changed, 54 insertions(+), 16 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java b/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java index 36478afaf54b..aeab237b98a9 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java @@ -17,11 +17,12 @@ import org.eclipse.jetty.http.BadMessageException; import org.eclipse.jetty.http.HttpStatus; +import org.eclipse.jetty.util.thread.AutoLock; import org.slf4j.Logger; import org.slf4j.LoggerFactory; /** - * Non-blocking {@link ContentProducer} implementation. Calling {@link #nextContent()} will never block + * Non-blocking {@link ContentProducer} implementation. Calling {@link ContentProducer#nextContent(AutoLock)} will never block * but will return null when there is no available content. */ class AsyncContentProducer implements ContentProducer @@ -192,8 +193,11 @@ public boolean onContentProducible() } @Override - public HttpInput.Content nextContent() + public HttpInput.Content nextContent(AutoLock lock) { + if (!lock.isHeldByCurrentThread()) + throw new IllegalStateException("nextContent must be called with the lock held"); + HttpInput.Content content = nextTransformedContent(); if (LOG.isDebugEnabled()) LOG.debug("nextContent = {} {}", content, this); diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/BlockingContentProducer.java b/jetty-server/src/main/java/org/eclipse/jetty/server/BlockingContentProducer.java index c25748edbb66..2ca1c9824686 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/BlockingContentProducer.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/BlockingContentProducer.java @@ -15,11 +15,12 @@ import java.util.concurrent.Semaphore; +import org.eclipse.jetty.util.thread.AutoLock; import org.slf4j.Logger; import org.slf4j.LoggerFactory; /** - * Blocking implementation of {@link ContentProducer}. Calling {@link #nextContent()} will block when + * Blocking implementation of {@link ContentProducer}. Calling {@link ContentProducer#nextContent(AutoLock)} will block when * there is no available content but will never return null. */ class BlockingContentProducer implements ContentProducer @@ -82,11 +83,11 @@ public boolean consumeAll(Throwable x) } @Override - public HttpInput.Content nextContent() + public HttpInput.Content nextContent(AutoLock lock) { while (true) { - HttpInput.Content content = _asyncContentProducer.nextContent(); + HttpInput.Content content = _asyncContentProducer.nextContent(lock); if (LOG.isDebugEnabled()) LOG.debug("nextContent async producer returned {}", content); if (content != null) @@ -103,14 +104,32 @@ public HttpInput.Content nextContent() if (LOG.isDebugEnabled()) LOG.debug("nextContent async producer is not ready, waiting on semaphore {}", _semaphore); + int lockReentranceCount = 0; try { + // Do not hold the lock during the wait on the semaphore. + // The lock must be unlocked more than once because it is + // reentrant so it fan be acquired multiple times by the + // same thread, so it can still be held by the thread after + // a single unlock call. + while (lock.isHeldByCurrentThread()) + { + lock.close(); + lockReentranceCount++; + } _semaphore.acquire(); } catch (InterruptedException e) { return new HttpInput.ErrorContent(e); } + finally + { + // Re-lock the lock as many times as it was held + // before the unlock preceding the semaphore acquisition. + for (int i = 0; i < lockReentranceCount; i++) + lock.lock(); + } } } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/ContentProducer.java b/jetty-server/src/main/java/org/eclipse/jetty/server/ContentProducer.java index 076619e816da..08649134e13c 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/ContentProducer.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/ContentProducer.java @@ -13,6 +13,8 @@ package org.eclipse.jetty.server; +import org.eclipse.jetty.util.thread.AutoLock; + /** * ContentProducer is the bridge between {@link HttpInput} and {@link HttpChannel}. * It wraps a {@link HttpChannel} and uses the {@link HttpChannel#needContent()}, @@ -94,8 +96,10 @@ public interface ContentProducer * After this call, state can be either of UNREADY or IDLE. * @return the next content that can be read from or null if the implementation does not block * and has no available content. + * @param lock The lock that is currently held. It will be released if this call has to block for the + * duration of the internal blocking. */ - HttpInput.Content nextContent(); + HttpInput.Content nextContent(AutoLock lock); /** * Free up the content by calling {@link HttpInput.Content#succeeded()} on it @@ -105,7 +109,7 @@ public interface ContentProducer /** * Check if this {@link ContentProducer} instance has some content that can be read without blocking. - * If there is some, the next call to {@link #nextContent()} will not block. + * If there is some, the next call to {@link #nextContent(AutoLock)} will not block. * If there isn't any and the implementation does not block, this method will trigger a * {@link javax.servlet.ReadListener} callback once some content is available. * This call is always non-blocking. 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 0b879d5ae215..94c0629b5c91 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 @@ -248,7 +248,7 @@ public int read(byte[] b, int off, int len) throws IOException // Calculate minimum request rate for DoS protection _contentProducer.checkMinDataRate(); - Content content = _contentProducer.nextContent(); + Content content = _contentProducer.nextContent(lock); if (content == null) throw new IllegalStateException("read on unready input"); if (!content.isSpecial()) @@ -341,7 +341,7 @@ public void run() LOG.debug("running but not ready {}", this); return; } - content = _contentProducer.nextContent(); + content = _contentProducer.nextContent(lock); if (LOG.isDebugEnabled()) LOG.debug("running on content {} {}", content, this); } diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/AsyncContentProducerTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/AsyncContentProducerTest.java index 19bd83da37b6..fb9d130ecea3 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/AsyncContentProducerTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/AsyncContentProducerTest.java @@ -28,8 +28,8 @@ import org.eclipse.jetty.io.ArrayByteBufferPool; import org.eclipse.jetty.io.EofException; import org.eclipse.jetty.server.handler.gzip.GzipHttpInputInterceptor; -import org.eclipse.jetty.util.compression.CompressionPool; import org.eclipse.jetty.util.compression.InflaterPool; +import org.eclipse.jetty.util.thread.AutoLock; import org.hamcrest.core.Is; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; @@ -176,6 +176,7 @@ private Throwable readAndAssertContent(int totalContentBytesCount, String origin int isReadyFalseCount = 0; int isReadyTrueCount = 0; Throwable error = null; + AutoLock lock = new AutoLock(); while (true) { @@ -184,13 +185,17 @@ private Throwable readAndAssertContent(int totalContentBytesCount, String origin else isReadyFalseCount++; - HttpInput.Content content = contentProducer.nextContent(); - nextContentCount++; - if (content == null) + HttpInput.Content content; + try (AutoLock autoLock = lock.lock()) { - barrier.await(5, TimeUnit.SECONDS); - content = contentProducer.nextContent(); + content = contentProducer.nextContent(lock); nextContentCount++; + if (content == null) + { + barrier.await(5, TimeUnit.SECONDS); + content = contentProducer.nextContent(lock); + nextContentCount++; + } } assertThat(content, notNullValue()); diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingContentProducerTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingContentProducerTest.java index 8755dc8c0e73..e32b01261d50 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingContentProducerTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingContentProducerTest.java @@ -27,6 +27,7 @@ import org.eclipse.jetty.io.EofException; import org.eclipse.jetty.server.handler.gzip.GzipHttpInputInterceptor; import org.eclipse.jetty.util.compression.InflaterPool; +import org.eclipse.jetty.util.thread.AutoLock; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -174,9 +175,14 @@ private Throwable readAndAssertContent(int totalContentBytesCount, String origin int nextContentCount = 0; String consumedString = ""; Throwable error = null; + AutoLock lock = new AutoLock(); while (true) { - HttpInput.Content content = contentProducer.nextContent(); + HttpInput.Content content; + try (AutoLock autoLock = lock.lock()) + { + content = contentProducer.nextContent(lock); + } nextContentCount++; if (content.isSpecial()) From 31eedec1ed01f1caf35d45ce2bdb2a5cc958416b Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Thu, 11 Feb 2021 16:21:33 +0100 Subject: [PATCH 29/33] internalize the lock within the content producer Signed-off-by: Ludovic Orban --- .../jetty/server/AsyncContentProducer.java | 33 +++++++++-- .../jetty/server/BlockingContentProducer.java | 29 ++++++---- .../eclipse/jetty/server/ContentProducer.java | 13 +++-- .../org/eclipse/jetty/server/HttpInput.java | 53 +++++++++--------- .../server/AsyncContentProducerTest.java | 56 +++++++++++-------- .../server/BlockingContentProducerTest.java | 50 ++++++++++------- 6 files changed, 146 insertions(+), 88 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java b/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java index aeab237b98a9..c915cc4fc7f5 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java @@ -22,13 +22,14 @@ import org.slf4j.LoggerFactory; /** - * Non-blocking {@link ContentProducer} implementation. Calling {@link ContentProducer#nextContent(AutoLock)} will never block + * Non-blocking {@link ContentProducer} implementation. Calling {@link ContentProducer#nextContent()} will never block * but will return null when there is no available content. */ class AsyncContentProducer implements ContentProducer { private static final Logger LOG = LoggerFactory.getLogger(AsyncContentProducer.class); + private final AutoLock _lock = new AutoLock(); private final HttpChannel _httpChannel; private HttpInput.Interceptor _interceptor; private HttpInput.Content _rawContent; @@ -42,9 +43,16 @@ class AsyncContentProducer implements ContentProducer _httpChannel = httpChannel; } + @Override + public AutoLock lock() + { + return _lock.lock(); + } + @Override public void recycle() { + assertLocked(); if (LOG.isDebugEnabled()) LOG.debug("recycling {}", this); _interceptor = null; @@ -58,18 +66,21 @@ public void recycle() @Override public HttpInput.Interceptor getInterceptor() { + assertLocked(); return _interceptor; } @Override public void setInterceptor(HttpInput.Interceptor interceptor) { + assertLocked(); this._interceptor = interceptor; } @Override public int available() { + assertLocked(); HttpInput.Content content = nextTransformedContent(); int available = content == null ? 0 : content.remaining(); if (LOG.isDebugEnabled()) @@ -80,6 +91,7 @@ public int available() @Override public boolean hasContent() { + assertLocked(); boolean hasContent = _rawContent != null; if (LOG.isDebugEnabled()) LOG.debug("hasContent = {} {}", hasContent, this); @@ -89,6 +101,7 @@ public boolean hasContent() @Override public boolean isError() { + assertLocked(); if (LOG.isDebugEnabled()) LOG.debug("isError = {} {}", _error, this); return _error; @@ -97,6 +110,7 @@ public boolean isError() @Override public void checkMinDataRate() { + assertLocked(); long minRequestDataRate = _httpChannel.getHttpConfiguration().getMinRequestDataRate(); if (LOG.isDebugEnabled()) LOG.debug("checkMinDataRate [m={},t={}] {}", minRequestDataRate, _firstByteTimeStamp, this); @@ -128,6 +142,7 @@ public void checkMinDataRate() @Override public long getRawContentArrived() { + assertLocked(); if (LOG.isDebugEnabled()) LOG.debug("getRawContentArrived = {} {}", _rawContentArrived, this); return _rawContentArrived; @@ -136,6 +151,7 @@ public long getRawContentArrived() @Override public boolean consumeAll(Throwable x) { + assertLocked(); if (LOG.isDebugEnabled()) LOG.debug("consumeAll [e={}] {}", x, this); failCurrentContent(x); @@ -187,17 +203,16 @@ private void failCurrentContent(Throwable x) @Override public boolean onContentProducible() { + assertLocked(); if (LOG.isDebugEnabled()) LOG.debug("onContentProducible {}", this); return _httpChannel.getState().onReadReady(); } @Override - public HttpInput.Content nextContent(AutoLock lock) + public HttpInput.Content nextContent() { - if (!lock.isHeldByCurrentThread()) - throw new IllegalStateException("nextContent must be called with the lock held"); - + assertLocked(); HttpInput.Content content = nextTransformedContent(); if (LOG.isDebugEnabled()) LOG.debug("nextContent = {} {}", content, this); @@ -209,6 +224,7 @@ public HttpInput.Content nextContent(AutoLock lock) @Override public void reclaim(HttpInput.Content content) { + assertLocked(); if (LOG.isDebugEnabled()) LOG.debug("reclaim {} {}", content, this); if (_transformedContent == content) @@ -223,6 +239,7 @@ public void reclaim(HttpInput.Content content) @Override public boolean isReady() { + assertLocked(); HttpInput.Content content = nextTransformedContent(); if (content != null) { @@ -367,6 +384,12 @@ private HttpInput.Content produceRawContent() return content; } + private void assertLocked() + { + if (!_lock.isHeldByCurrentThread()) + throw new IllegalStateException("nextContent must be called within a critical block"); + } + @Override public String toString() { diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/BlockingContentProducer.java b/jetty-server/src/main/java/org/eclipse/jetty/server/BlockingContentProducer.java index 2ca1c9824686..18acf913911a 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/BlockingContentProducer.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/BlockingContentProducer.java @@ -20,7 +20,7 @@ import org.slf4j.LoggerFactory; /** - * Blocking implementation of {@link ContentProducer}. Calling {@link ContentProducer#nextContent(AutoLock)} will block when + * Blocking implementation of {@link ContentProducer}. Calling {@link ContentProducer#nextContent()} will block when * there is no available content but will never return null. */ class BlockingContentProducer implements ContentProducer @@ -35,6 +35,12 @@ class BlockingContentProducer implements ContentProducer _asyncContentProducer = delegate; } + @Override + public AutoLock lock() + { + return _asyncContentProducer.lock(); + } + @Override public void recycle() { @@ -83,11 +89,11 @@ public boolean consumeAll(Throwable x) } @Override - public HttpInput.Content nextContent(AutoLock lock) + public HttpInput.Content nextContent() { while (true) { - HttpInput.Content content = _asyncContentProducer.nextContent(lock); + HttpInput.Content content = _asyncContentProducer.nextContent(); if (LOG.isDebugEnabled()) LOG.debug("nextContent async producer returned {}", content); if (content != null) @@ -104,18 +110,19 @@ public HttpInput.Content nextContent(AutoLock lock) if (LOG.isDebugEnabled()) LOG.debug("nextContent async producer is not ready, waiting on semaphore {}", _semaphore); - int lockReentranceCount = 0; + AutoLock lock = _asyncContentProducer.lock(); + // Start the counter at -1 b/c acquiring the lock increases the hold count by 1. + int lockHoldCount = -1; try { - // Do not hold the lock during the wait on the semaphore. - // The lock must be unlocked more than once because it is - // reentrant so it fan be acquired multiple times by the - // same thread, so it can still be held by the thread after - // a single unlock call. + // Do not hold the lock during the wait on the semaphore; + // the lock must be unlocked more than once because it is + // reentrant so it can be acquired multiple times by the same + // thread, hence it can still be held after a single unlock. while (lock.isHeldByCurrentThread()) { lock.close(); - lockReentranceCount++; + lockHoldCount++; } _semaphore.acquire(); } @@ -127,7 +134,7 @@ public HttpInput.Content nextContent(AutoLock lock) { // Re-lock the lock as many times as it was held // before the unlock preceding the semaphore acquisition. - for (int i = 0; i < lockReentranceCount; i++) + for (int i = 0; i < lockHoldCount; i++) lock.lock(); } } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/ContentProducer.java b/jetty-server/src/main/java/org/eclipse/jetty/server/ContentProducer.java index 08649134e13c..533a041ca1ad 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/ContentProducer.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/ContentProducer.java @@ -26,6 +26,13 @@ */ public interface ContentProducer { + /** + * Lock this instance. The lock must be held before any method of this instance's + * method be called, and must be manually released afterward. + * @return the lock that is guarding this instance. + */ + AutoLock lock(); + /** * Reset all internal state and clear any held resources. */ @@ -96,10 +103,8 @@ public interface ContentProducer * After this call, state can be either of UNREADY or IDLE. * @return the next content that can be read from or null if the implementation does not block * and has no available content. - * @param lock The lock that is currently held. It will be released if this call has to block for the - * duration of the internal blocking. */ - HttpInput.Content nextContent(AutoLock lock); + HttpInput.Content nextContent(); /** * Free up the content by calling {@link HttpInput.Content#succeeded()} on it @@ -109,7 +114,7 @@ public interface ContentProducer /** * Check if this {@link ContentProducer} instance has some content that can be read without blocking. - * If there is some, the next call to {@link #nextContent(AutoLock)} will not block. + * If there is some, the next call to {@link #nextContent()} will not block. * If there isn't any and the implementation does not block, this method will trigger a * {@link javax.servlet.ReadListener} callback once some content is available. * This call is always non-blocking. 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 94c0629b5c91..d7eee8efe7b3 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 @@ -39,7 +39,6 @@ public class HttpInput extends ServletInputStream implements Runnable private final byte[] _oneByteBuffer = new byte[1]; private final BlockingContentProducer _blockingContentProducer; private final AsyncContentProducer _asyncContentProducer; - private final AutoLock _contentProducerLock = new AutoLock(); private final HttpChannelState _channelState; private final LongAdder _contentConsumed = new LongAdder(); private volatile ContentProducer _contentProducer; @@ -62,13 +61,16 @@ public void recycle() public void reopen() { - if (LOG.isDebugEnabled()) - LOG.debug("reopen {}", this); - _blockingContentProducer.recycle(); - _contentProducer = _blockingContentProducer; - _consumedEof = false; - _readListener = null; - _contentConsumed.reset(); + try (AutoLock lock = _contentProducer.lock()) + { + if (LOG.isDebugEnabled()) + LOG.debug("reopen {}", this); + _blockingContentProducer.recycle(); + _contentProducer = _blockingContentProducer; + _consumedEof = false; + _readListener = null; + _contentConsumed.reset(); + } } /** @@ -76,7 +78,7 @@ public void reopen() */ public Interceptor getInterceptor() { - try (AutoLock lock = _contentProducerLock.lock()) + try (AutoLock lock = _contentProducer.lock()) { return _contentProducer.getInterceptor(); } @@ -89,7 +91,7 @@ public Interceptor getInterceptor() */ public void setInterceptor(Interceptor interceptor) { - try (AutoLock lock = _contentProducerLock.lock()) + try (AutoLock lock = _contentProducer.lock()) { if (LOG.isDebugEnabled()) LOG.debug("setting interceptor to {} on {}", interceptor, this); @@ -105,7 +107,7 @@ public void setInterceptor(Interceptor interceptor) */ public void addInterceptor(Interceptor interceptor) { - try (AutoLock lock = _contentProducerLock.lock()) + try (AutoLock lock = _contentProducer.lock()) { Interceptor currentInterceptor = _contentProducer.getInterceptor(); if (currentInterceptor == null) @@ -138,7 +140,7 @@ public long getContentConsumed() public long getContentReceived() { - try (AutoLock lock = _contentProducerLock.lock()) + try (AutoLock lock = _contentProducer.lock()) { return _contentProducer.getRawContentArrived(); } @@ -146,7 +148,7 @@ public long getContentReceived() public boolean consumeAll() { - try (AutoLock lock = _contentProducerLock.lock()) + try (AutoLock lock = _contentProducer.lock()) { IOException failure = new IOException("Unconsumed content"); if (LOG.isDebugEnabled()) @@ -164,7 +166,7 @@ public boolean consumeAll() public boolean isError() { - try (AutoLock lock = _contentProducerLock.lock()) + try (AutoLock lock = _contentProducer.lock()) { boolean error = _contentProducer.isError(); if (LOG.isDebugEnabled()) @@ -194,7 +196,7 @@ public boolean isFinished() @Override public boolean isReady() { - try (AutoLock lock = _contentProducerLock.lock()) + try (AutoLock lock = _contentProducer.lock()) { boolean ready = _contentProducer.isReady(); if (LOG.isDebugEnabled()) @@ -223,15 +225,16 @@ public void setReadListener(ReadListener readListener) public boolean onContentProducible() { - // This is the only _contentProducer method call that must not - // happen under a lock as it is used to release blocking calls. - return _contentProducer.onContentProducible(); + try (AutoLock lock = _contentProducer.lock()) + { + return _contentProducer.onContentProducible(); + } } @Override public int read() throws IOException { - try (AutoLock lock = _contentProducerLock.lock()) + try (AutoLock lock = _contentProducer.lock()) { int read = read(_oneByteBuffer, 0, 1); if (read == 0) @@ -243,12 +246,12 @@ public int read() throws IOException @Override public int read(byte[] b, int off, int len) throws IOException { - try (AutoLock lock = _contentProducerLock.lock()) + try (AutoLock lock = _contentProducer.lock()) { // Calculate minimum request rate for DoS protection _contentProducer.checkMinDataRate(); - Content content = _contentProducer.nextContent(lock); + Content content = _contentProducer.nextContent(); if (content == null) throw new IllegalStateException("read on unready input"); if (!content.isSpecial()) @@ -299,7 +302,7 @@ private void scheduleReadListenerNotification() */ public boolean hasContent() { - try (AutoLock lock = _contentProducerLock.lock()) + try (AutoLock lock = _contentProducer.lock()) { // Do not call _contentProducer.available() as it calls HttpChannel.produceContent() // which is forbidden by this method's contract. @@ -313,7 +316,7 @@ public boolean hasContent() @Override public int available() { - try (AutoLock lock = _contentProducerLock.lock()) + try (AutoLock lock = _contentProducer.lock()) { int available = _contentProducer.available(); if (LOG.isDebugEnabled()) @@ -332,7 +335,7 @@ public int available() public void run() { Content content; - try (AutoLock lock = _contentProducerLock.lock()) + try (AutoLock lock = _contentProducer.lock()) { // Call isReady() to make sure that if not ready we register for fill interest. if (!_contentProducer.isReady()) @@ -341,7 +344,7 @@ public void run() LOG.debug("running but not ready {}", this); return; } - content = _contentProducer.nextContent(lock); + content = _contentProducer.nextContent(); if (LOG.isDebugEnabled()) LOG.debug("running on content {} {}", content, this); } diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/AsyncContentProducerTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/AsyncContentProducerTest.java index fb9d130ecea3..047a53827551 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/AsyncContentProducerTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/AsyncContentProducerTest.java @@ -72,8 +72,11 @@ public void testAsyncContentProducerNoInterceptor() throws Exception ContentProducer contentProducer = new AsyncContentProducer(new ArrayDelayedHttpChannel(buffers, new HttpInput.EofContent(), scheduledExecutorService, barrier)); - Throwable error = readAndAssertContent(totalContentBytesCount, originalContentString, contentProducer, (buffers.length + 1) * 2, 0, 4, barrier); - assertThat(error, nullValue()); + try (AutoLock lock = contentProducer.lock()) + { + Throwable error = readAndAssertContent(totalContentBytesCount, originalContentString, contentProducer, (buffers.length + 1) * 2, 0, 4, barrier); + assertThat(error, nullValue()); + } } @Test @@ -91,8 +94,11 @@ public void testAsyncContentProducerNoInterceptorWithError() throws Exception ContentProducer contentProducer = new AsyncContentProducer(new ArrayDelayedHttpChannel(buffers, new HttpInput.ErrorContent(expectedError), scheduledExecutorService, barrier)); - Throwable error = readAndAssertContent(totalContentBytesCount, originalContentString, contentProducer, (buffers.length + 1) * 2, 0, 4, barrier); - assertThat(error, Is.is(expectedError)); + try (AutoLock lock = contentProducer.lock()) + { + Throwable error = readAndAssertContent(totalContentBytesCount, originalContentString, contentProducer, (buffers.length + 1) * 2, 0, 4, barrier); + assertThat(error, Is.is(expectedError)); + } } @Test @@ -113,10 +119,13 @@ public void testAsyncContentProducerGzipInterceptor() throws Exception CyclicBarrier barrier = new CyclicBarrier(2); ContentProducer contentProducer = new AsyncContentProducer(new ArrayDelayedHttpChannel(buffers, new HttpInput.EofContent(), scheduledExecutorService, barrier)); - contentProducer.setInterceptor(new GzipHttpInputInterceptor(inflaterPool, new ArrayByteBufferPool(1, 1, 2), 32)); + try (AutoLock lock = contentProducer.lock()) + { + contentProducer.setInterceptor(new GzipHttpInputInterceptor(inflaterPool, new ArrayByteBufferPool(1, 1, 2), 32)); - Throwable error = readAndAssertContent(totalContentBytesCount, originalContentString, contentProducer, (buffers.length + 1) * 2, 0, 4, barrier); - assertThat(error, nullValue()); + Throwable error = readAndAssertContent(totalContentBytesCount, originalContentString, contentProducer, (buffers.length + 1) * 2, 0, 4, barrier); + assertThat(error, nullValue()); + } } @Test @@ -137,10 +146,13 @@ public void testAsyncContentProducerGzipInterceptorWithTinyBuffers() throws Exce CyclicBarrier barrier = new CyclicBarrier(2); ContentProducer contentProducer = new AsyncContentProducer(new ArrayDelayedHttpChannel(buffers, new HttpInput.EofContent(), scheduledExecutorService, barrier)); - contentProducer.setInterceptor(new GzipHttpInputInterceptor(inflaterPool, new ArrayByteBufferPool(1, 1, 2), 1)); + try (AutoLock lock = contentProducer.lock()) + { + contentProducer.setInterceptor(new GzipHttpInputInterceptor(inflaterPool, new ArrayByteBufferPool(1, 1, 2), 1)); - Throwable error = readAndAssertContent(totalContentBytesCount, originalContentString, contentProducer, totalContentBytesCount + buffers.length + 2, 25, 4, barrier); - assertThat(error, nullValue()); + Throwable error = readAndAssertContent(totalContentBytesCount, originalContentString, contentProducer, totalContentBytesCount + buffers.length + 2, 25, 4, barrier); + assertThat(error, nullValue()); + } } @Test @@ -162,10 +174,13 @@ public void testBlockingContentProducerGzipInterceptorWithError() throws Excepti CyclicBarrier barrier = new CyclicBarrier(2); ContentProducer contentProducer = new AsyncContentProducer(new ArrayDelayedHttpChannel(buffers, new HttpInput.ErrorContent(expectedError), scheduledExecutorService, barrier)); - contentProducer.setInterceptor(new GzipHttpInputInterceptor(inflaterPool, new ArrayByteBufferPool(1, 1, 2), 32)); + try (AutoLock lock = contentProducer.lock()) + { + contentProducer.setInterceptor(new GzipHttpInputInterceptor(inflaterPool, new ArrayByteBufferPool(1, 1, 2), 32)); - Throwable error = readAndAssertContent(totalContentBytesCount, originalContentString, contentProducer, (buffers.length + 1) * 2, 0, 4, barrier); - assertThat(error, Is.is(expectedError)); + Throwable error = readAndAssertContent(totalContentBytesCount, originalContentString, contentProducer, (buffers.length + 1) * 2, 0, 4, barrier); + assertThat(error, Is.is(expectedError)); + } } private Throwable readAndAssertContent(int totalContentBytesCount, String originalContentString, ContentProducer contentProducer, int totalContentCount, int readyCount, int notReadyCount, CyclicBarrier barrier) throws InterruptedException, BrokenBarrierException, TimeoutException @@ -176,7 +191,6 @@ private Throwable readAndAssertContent(int totalContentBytesCount, String origin int isReadyFalseCount = 0; int isReadyTrueCount = 0; Throwable error = null; - AutoLock lock = new AutoLock(); while (true) { @@ -185,17 +199,13 @@ private Throwable readAndAssertContent(int totalContentBytesCount, String origin else isReadyFalseCount++; - HttpInput.Content content; - try (AutoLock autoLock = lock.lock()) + HttpInput.Content content = contentProducer.nextContent(); + nextContentCount++; + if (content == null) { - content = contentProducer.nextContent(lock); + barrier.await(5, TimeUnit.SECONDS); + content = contentProducer.nextContent(); nextContentCount++; - if (content == null) - { - barrier.await(5, TimeUnit.SECONDS); - content = contentProducer.nextContent(lock); - nextContentCount++; - } } assertThat(content, notNullValue()); diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingContentProducerTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingContentProducerTest.java index e32b01261d50..304472ae0035 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingContentProducerTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingContentProducerTest.java @@ -69,8 +69,11 @@ public void testBlockingContentProducerNoInterceptor() ContentProducer contentProducer = new BlockingContentProducer(new AsyncContentProducer(httpChannel)); ref.set(contentProducer); - Throwable error = readAndAssertContent(totalContentBytesCount, originalContentString, buffers.length + 1, contentProducer); - assertThat(error, nullValue()); + try (AutoLock lock = contentProducer.lock()) + { + Throwable error = readAndAssertContent(totalContentBytesCount, originalContentString, buffers.length + 1, contentProducer); + assertThat(error, nullValue()); + } } @Test @@ -87,10 +90,13 @@ public void testBlockingContentProducerNoInterceptorWithError() AtomicReference ref = new AtomicReference<>(); ArrayDelayedHttpChannel httpChannel = new ArrayDelayedHttpChannel(buffers, new HttpInput.ErrorContent(expectedError), scheduledExecutorService, () -> ref.get().onContentProducible()); ContentProducer contentProducer = new BlockingContentProducer(new AsyncContentProducer(httpChannel)); - ref.set(contentProducer); + try (AutoLock lock = contentProducer.lock()) + { + ref.set(contentProducer); - Throwable error = readAndAssertContent(totalContentBytesCount, originalContentString, buffers.length + 1, contentProducer); - assertThat(error, is(expectedError)); + Throwable error = readAndAssertContent(totalContentBytesCount, originalContentString, buffers.length + 1, contentProducer); + assertThat(error, is(expectedError)); + } } @Test @@ -112,10 +118,13 @@ public void testBlockingContentProducerGzipInterceptor() ArrayDelayedHttpChannel httpChannel = new ArrayDelayedHttpChannel(buffers, new HttpInput.EofContent(), scheduledExecutorService, () -> ref.get().onContentProducible()); ContentProducer contentProducer = new BlockingContentProducer(new AsyncContentProducer(httpChannel)); ref.set(contentProducer); - contentProducer.setInterceptor(new GzipHttpInputInterceptor(inflaterPool, new ArrayByteBufferPool(1, 1, 2), 32)); + try (AutoLock lock = contentProducer.lock()) + { + contentProducer.setInterceptor(new GzipHttpInputInterceptor(inflaterPool, new ArrayByteBufferPool(1, 1, 2), 32)); - Throwable error = readAndAssertContent(totalContentBytesCount, originalContentString, buffers.length + 1, contentProducer); - assertThat(error, nullValue()); + Throwable error = readAndAssertContent(totalContentBytesCount, originalContentString, buffers.length + 1, contentProducer); + assertThat(error, nullValue()); + } } @Test @@ -137,10 +146,13 @@ public void testBlockingContentProducerGzipInterceptorWithTinyBuffers() ArrayDelayedHttpChannel httpChannel = new ArrayDelayedHttpChannel(buffers, new HttpInput.EofContent(), scheduledExecutorService, () -> ref.get().onContentProducible()); ContentProducer contentProducer = new BlockingContentProducer(new AsyncContentProducer(httpChannel)); ref.set(contentProducer); - contentProducer.setInterceptor(new GzipHttpInputInterceptor(inflaterPool, new ArrayByteBufferPool(1, 1, 2), 1)); + try (AutoLock lock = contentProducer.lock()) + { + contentProducer.setInterceptor(new GzipHttpInputInterceptor(inflaterPool, new ArrayByteBufferPool(1, 1, 2), 1)); - Throwable error = readAndAssertContent(totalContentBytesCount, originalContentString, totalContentBytesCount + 1, contentProducer); - assertThat(error, nullValue()); + Throwable error = readAndAssertContent(totalContentBytesCount, originalContentString, totalContentBytesCount + 1, contentProducer); + assertThat(error, nullValue()); + } } @Test @@ -163,10 +175,13 @@ public void testBlockingContentProducerGzipInterceptorWithError() ArrayDelayedHttpChannel httpChannel = new ArrayDelayedHttpChannel(buffers, new HttpInput.ErrorContent(expectedError), scheduledExecutorService, () -> ref.get().onContentProducible()); ContentProducer contentProducer = new BlockingContentProducer(new AsyncContentProducer(httpChannel)); ref.set(contentProducer); - contentProducer.setInterceptor(new GzipHttpInputInterceptor(inflaterPool, new ArrayByteBufferPool(1, 1, 2), 32)); + try (AutoLock lock = contentProducer.lock()) + { + contentProducer.setInterceptor(new GzipHttpInputInterceptor(inflaterPool, new ArrayByteBufferPool(1, 1, 2), 32)); - Throwable error = readAndAssertContent(totalContentBytesCount, originalContentString, buffers.length + 1, contentProducer); - assertThat(error, is(expectedError)); + Throwable error = readAndAssertContent(totalContentBytesCount, originalContentString, buffers.length + 1, contentProducer); + assertThat(error, is(expectedError)); + } } private Throwable readAndAssertContent(int totalContentBytesCount, String originalContentString, int totalContentCount, ContentProducer contentProducer) @@ -175,14 +190,9 @@ private Throwable readAndAssertContent(int totalContentBytesCount, String origin int nextContentCount = 0; String consumedString = ""; Throwable error = null; - AutoLock lock = new AutoLock(); while (true) { - HttpInput.Content content; - try (AutoLock autoLock = lock.lock()) - { - content = contentProducer.nextContent(lock); - } + HttpInput.Content content = contentProducer.nextContent(); nextContentCount++; if (content.isSpecial()) From 8049be5fcb84edbf02d63438db044ad9807495bc Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Fri, 12 Feb 2021 17:11:38 +0100 Subject: [PATCH 30/33] repro test Signed-off-by: Ludovic Orban --- .../eclipse/jetty/server/BlockingTest.java | 109 +++++++++++++++++- 1 file changed, 108 insertions(+), 1 deletion(-) diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingTest.java index f1f45c90aa78..2934fc5b0afe 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingTest.java @@ -14,6 +14,7 @@ package org.eclipse.jetty.server; import java.io.BufferedReader; +import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStreamReader; import java.io.OutputStream; @@ -22,12 +23,16 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.concurrent.BrokenBarrierException; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.CyclicBarrier; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; +import java.util.zip.GZIPOutputStream; import javax.servlet.AsyncContext; import javax.servlet.DispatcherType; import javax.servlet.ServletException; +import javax.servlet.ServletOutputStream; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -35,9 +40,12 @@ import org.eclipse.jetty.server.handler.AbstractHandler; import org.eclipse.jetty.server.handler.ContextHandler; import org.eclipse.jetty.server.handler.DefaultHandler; +import org.eclipse.jetty.server.handler.HandlerCollection; import org.eclipse.jetty.server.handler.HandlerList; +import org.eclipse.jetty.server.handler.gzip.GzipHandler; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import static org.hamcrest.MatcherAssert.assertThat; @@ -59,7 +67,7 @@ void setUp() { server = new Server(); connector = new ServerConnector(server); - connector.setPort(0); + connector.setPort(8888); server.addConnector(connector); context = new ContextHandler("/ctx"); @@ -152,6 +160,105 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques } } + @Test + @Disabled("broken because of 5605") + public void testBlockingReadAndBlockingWriteGzipped() throws Exception + { + AtomicReference threadRef = new AtomicReference<>(); + CyclicBarrier barrier = new CyclicBarrier(2); + + AbstractHandler handler = new AbstractHandler() + { + @Override + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + { + try + { + baseRequest.setHandled(true); + final AsyncContext asyncContext = baseRequest.startAsync(); + final ServletOutputStream outputStream = response.getOutputStream(); + final Thread thread = new Thread(() -> + { + try + { + for (int i = 0; i < 5; i++) + { + int b = baseRequest.getHttpInput().read(); + System.out.println(b); + } + System.out.println("handler read bytes"); + outputStream.write("All read.".getBytes(StandardCharsets.UTF_8)); + System.out.println("handler wrote bytes"); + barrier.await(); // notify that all bytes were read + baseRequest.getHttpInput().read(); // this read should throw IOException as the client has closed the connection + throw new AssertionError("should have thrown IOException"); + } + catch (Exception e) + { + throw new RuntimeException(e); + } + finally + { + try + { + outputStream.close(); + } + catch (Exception e2) + { + e2.printStackTrace(); + } + asyncContext.complete(); + } + }); + threadRef.set(thread); + thread.start(); + barrier.await(); // notify that handler thread has started + + response.setStatus(200); + response.setContentType("text/plain"); + response.getOutputStream().print("OK\r\n"); + } + catch (Exception e) + { + throw new RuntimeException(e); + } + } + }; + GzipHandler gzipHandler = new GzipHandler(); + gzipHandler.setMinGzipSize(1); + gzipHandler.setHandler(handler); + context.setHandler(gzipHandler); + // using the GzipHandler is mandatory to reproduce the +// context.setHandler(handler); + server.start(); + + StringBuilder request = new StringBuilder(); + // partial chunked request + request.append("POST /ctx/path/info HTTP/1.1\r\n") + .append("Host: localhost\r\n") + .append("Accept-Encoding: gzip, *\r\n") + .append("Content-Type: test/data\r\n") + .append("Transfer-Encoding: chunked\r\n") + .append("\r\n") + .append("10\r\n") + .append("01234") + ; + + int port = connector.getLocalPort(); + try (Socket socket = new Socket("localhost", port)) + { + socket.setSoLinger(true, 0); // send TCP RST upon close instead of FIN + OutputStream out = socket.getOutputStream(); + out.write(request.toString().getBytes(StandardCharsets.ISO_8859_1)); + System.out.println("request sent"); + barrier.await(); // wait for handler thread to be started + barrier.await(); // wait for all bytes of the request to be read + } + System.out.println("client connection closed"); + threadRef.get().join(5000); + assertThat("handler thread should not be alive anymore", threadRef.get().isAlive(), is(false)); + } + @Test public void testNormalCompleteThenBlockingRead() throws Exception { From bf9318f2b8a53c8d8c1d572ad7f3e982e2613c82 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Fri, 12 Feb 2021 17:43:05 +0100 Subject: [PATCH 31/33] fix HttpOutput.close() hanging forever when client sends TCP RST and GZIP is enabled Signed-off-by: Ludovic Orban --- .../handler/gzip/GzipHttpOutputInterceptor.java | 7 +++++-- .../org/eclipse/jetty/server/BlockingTest.java | 16 +++++++--------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHttpOutputInterceptor.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHttpOutputInterceptor.java index fe5b816db325..be33871a9891 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHttpOutputInterceptor.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHttpOutputInterceptor.java @@ -273,8 +273,11 @@ public GzipBufferCB(ByteBuffer content, boolean complete, Callback callback) @Override protected void onCompleteFailure(Throwable x) { - _deflaterEntry.release(); - _deflaterEntry = null; + if (_deflaterEntry != null) + { + _deflaterEntry.release(); + _deflaterEntry = null; + } super.onCompleteFailure(x); } diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingTest.java index 2934fc5b0afe..ac527e8cfa0b 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingTest.java @@ -28,6 +28,7 @@ import java.util.concurrent.CyclicBarrier; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Predicate; import java.util.zip.GZIPOutputStream; import javax.servlet.AsyncContext; import javax.servlet.DispatcherType; @@ -43,6 +44,8 @@ import org.eclipse.jetty.server.handler.HandlerCollection; import org.eclipse.jetty.server.handler.HandlerList; import org.eclipse.jetty.server.handler.gzip.GzipHandler; +import org.hamcrest.Matchers; +import org.hamcrest.core.Is; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Disabled; @@ -51,6 +54,7 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.instanceOf; +import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.startsWith; import static org.hamcrest.core.Is.is; @@ -67,7 +71,6 @@ void setUp() { server = new Server(); connector = new ServerConnector(server); - connector.setPort(8888); server.addConnector(connector); context = new ContextHandler("/ctx"); @@ -161,7 +164,6 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques } @Test - @Disabled("broken because of 5605") public void testBlockingReadAndBlockingWriteGzipped() throws Exception { AtomicReference threadRef = new AtomicReference<>(); @@ -184,18 +186,16 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques for (int i = 0; i < 5; i++) { int b = baseRequest.getHttpInput().read(); - System.out.println(b); + assertThat(b, not(is(-1))); } - System.out.println("handler read bytes"); outputStream.write("All read.".getBytes(StandardCharsets.UTF_8)); - System.out.println("handler wrote bytes"); barrier.await(); // notify that all bytes were read baseRequest.getHttpInput().read(); // this read should throw IOException as the client has closed the connection throw new AssertionError("should have thrown IOException"); } catch (Exception e) { - throw new RuntimeException(e); + //throw new RuntimeException(e); } finally { @@ -205,7 +205,7 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques } catch (Exception e2) { - e2.printStackTrace(); + //e2.printStackTrace(); } asyncContext.complete(); } @@ -250,11 +250,9 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques socket.setSoLinger(true, 0); // send TCP RST upon close instead of FIN OutputStream out = socket.getOutputStream(); out.write(request.toString().getBytes(StandardCharsets.ISO_8859_1)); - System.out.println("request sent"); barrier.await(); // wait for handler thread to be started barrier.await(); // wait for all bytes of the request to be read } - System.out.println("client connection closed"); threadRef.get().join(5000); assertThat("handler thread should not be alive anymore", threadRef.get().isAlive(), is(false)); } From 4ec51fbbcafbc720808342d6702bbedf816d5a25 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Mon, 15 Feb 2021 11:28:10 +0100 Subject: [PATCH 32/33] abort when onError() fails Signed-off-by: Ludovic Orban --- .../java/org/eclipse/jetty/server/HttpChannel.java | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) 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 a7c1eb35d4f3..e4f5d275f1fe 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 @@ -701,9 +701,20 @@ else if (noStack != null) } if (isCommitted()) + { abort(failure); + } else - _state.onError(failure); + { + try + { + _state.onError(failure); + } + catch (IllegalStateException e) + { + abort(failure); + } + } } /** From f70a76651d33fffc68894aee8ae9b95498c0e8a2 Mon Sep 17 00:00:00 2001 From: Ludovic Orban Date: Mon, 15 Feb 2021 19:29:21 +0100 Subject: [PATCH 33/33] replace semaphore with cond variable + counter Signed-off-by: Ludovic Orban --- .../jetty/server/AsyncContentProducer.java | 52 ++++++++++++++++- .../jetty/server/BlockingContentProducer.java | 25 +------- .../server/BlockingContentProducerTest.java | 57 ++++++++++++------- 3 files changed, 92 insertions(+), 42 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java b/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java index c915cc4fc7f5..72202727e9fa 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java @@ -14,6 +14,7 @@ package org.eclipse.jetty.server; import java.util.concurrent.TimeUnit; +import java.util.concurrent.locks.Condition; import org.eclipse.jetty.http.BadMessageException; import org.eclipse.jetty.http.HttpStatus; @@ -387,7 +388,7 @@ private HttpInput.Content produceRawContent() private void assertLocked() { if (!_lock.isHeldByCurrentThread()) - throw new IllegalStateException("nextContent must be called within a critical block"); + throw new IllegalStateException("ContentProducer must be called within lock scope"); } @Override @@ -403,4 +404,53 @@ public String toString() _httpChannel ); } + + LockedSemaphore newLockedSemaphore() + { + return new LockedSemaphore(); + } + + /** + * A semaphore that assumes working under {@link AsyncContentProducer#lock()} scope. + */ + class LockedSemaphore + { + private final Condition _condition; + private int _permits; + + private LockedSemaphore() + { + this._condition = _lock.newCondition(); + } + + void assertLocked() + { + if (!_lock.isHeldByCurrentThread()) + throw new IllegalStateException("LockedSemaphore must be called within lock scope"); + } + + void drainPermits() + { + _permits = 0; + } + + void acquire() throws InterruptedException + { + while (_permits == 0) + _condition.await(); + _permits--; + } + + void release() + { + _permits++; + _condition.signal(); + } + + @Override + public String toString() + { + return getClass().getSimpleName() + " permits=" + _permits; + } + } } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/BlockingContentProducer.java b/jetty-server/src/main/java/org/eclipse/jetty/server/BlockingContentProducer.java index 18acf913911a..c525de21d4e2 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/BlockingContentProducer.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/BlockingContentProducer.java @@ -13,8 +13,6 @@ package org.eclipse.jetty.server; -import java.util.concurrent.Semaphore; - import org.eclipse.jetty.util.thread.AutoLock; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -27,12 +25,13 @@ class BlockingContentProducer implements ContentProducer { private static final Logger LOG = LoggerFactory.getLogger(BlockingContentProducer.class); - private final Semaphore _semaphore = new Semaphore(0); private final AsyncContentProducer _asyncContentProducer; + private final AsyncContentProducer.LockedSemaphore _semaphore; BlockingContentProducer(AsyncContentProducer delegate) { _asyncContentProducer = delegate; + _semaphore = _asyncContentProducer.newLockedSemaphore(); } @Override @@ -110,33 +109,14 @@ public HttpInput.Content nextContent() if (LOG.isDebugEnabled()) LOG.debug("nextContent async producer is not ready, waiting on semaphore {}", _semaphore); - AutoLock lock = _asyncContentProducer.lock(); - // Start the counter at -1 b/c acquiring the lock increases the hold count by 1. - int lockHoldCount = -1; try { - // Do not hold the lock during the wait on the semaphore; - // the lock must be unlocked more than once because it is - // reentrant so it can be acquired multiple times by the same - // thread, hence it can still be held after a single unlock. - while (lock.isHeldByCurrentThread()) - { - lock.close(); - lockHoldCount++; - } _semaphore.acquire(); } catch (InterruptedException e) { return new HttpInput.ErrorContent(e); } - finally - { - // Re-lock the lock as many times as it was held - // before the unlock preceding the semaphore acquisition. - for (int i = 0; i < lockHoldCount; i++) - lock.lock(); - } } } @@ -170,6 +150,7 @@ public void setInterceptor(HttpInput.Interceptor interceptor) @Override public boolean onContentProducible() { + _semaphore.assertLocked(); // In blocking mode, the dispatched thread normally does not have to be rescheduled as it is normally in state // DISPATCHED blocked on the semaphore that just needs to be released for the dispatched thread to resume. This is why // this method always returns false. diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingContentProducerTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingContentProducerTest.java index 304472ae0035..8cc7ca8b523b 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingContentProducerTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/BlockingContentProducerTest.java @@ -20,7 +20,6 @@ import java.util.concurrent.Executors; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicReference; import java.util.zip.GZIPOutputStream; import org.eclipse.jetty.io.ArrayByteBufferPool; @@ -64,10 +63,10 @@ public void testBlockingContentProducerNoInterceptor() final int totalContentBytesCount = countRemaining(buffers); final String originalContentString = asString(buffers); - AtomicReference ref = new AtomicReference<>(); - ArrayDelayedHttpChannel httpChannel = new ArrayDelayedHttpChannel(buffers, new HttpInput.EofContent(), scheduledExecutorService, () -> ref.get().onContentProducible()); + ContentListener contentListener = new ContentListener(); + ArrayDelayedHttpChannel httpChannel = new ArrayDelayedHttpChannel(buffers, new HttpInput.EofContent(), scheduledExecutorService, contentListener); ContentProducer contentProducer = new BlockingContentProducer(new AsyncContentProducer(httpChannel)); - ref.set(contentProducer); + contentListener.setContentProducer(contentProducer); try (AutoLock lock = contentProducer.lock()) { @@ -87,13 +86,13 @@ public void testBlockingContentProducerNoInterceptorWithError() final String originalContentString = asString(buffers); final Throwable expectedError = new EofException("Early EOF"); - AtomicReference ref = new AtomicReference<>(); - ArrayDelayedHttpChannel httpChannel = new ArrayDelayedHttpChannel(buffers, new HttpInput.ErrorContent(expectedError), scheduledExecutorService, () -> ref.get().onContentProducible()); + ContentListener contentListener = new ContentListener(); + ArrayDelayedHttpChannel httpChannel = new ArrayDelayedHttpChannel(buffers, new HttpInput.ErrorContent(expectedError), scheduledExecutorService, contentListener); ContentProducer contentProducer = new BlockingContentProducer(new AsyncContentProducer(httpChannel)); + contentListener.setContentProducer(contentProducer); + try (AutoLock lock = contentProducer.lock()) { - ref.set(contentProducer); - Throwable error = readAndAssertContent(totalContentBytesCount, originalContentString, buffers.length + 1, contentProducer); assertThat(error, is(expectedError)); } @@ -114,10 +113,11 @@ public void testBlockingContentProducerGzipInterceptor() buffers[1] = gzipByteBuffer(uncompressedBuffers[1]); buffers[2] = gzipByteBuffer(uncompressedBuffers[2]); - AtomicReference ref = new AtomicReference<>(); - ArrayDelayedHttpChannel httpChannel = new ArrayDelayedHttpChannel(buffers, new HttpInput.EofContent(), scheduledExecutorService, () -> ref.get().onContentProducible()); + ContentListener contentListener = new ContentListener(); + ArrayDelayedHttpChannel httpChannel = new ArrayDelayedHttpChannel(buffers, new HttpInput.EofContent(), scheduledExecutorService, contentListener); ContentProducer contentProducer = new BlockingContentProducer(new AsyncContentProducer(httpChannel)); - ref.set(contentProducer); + contentListener.setContentProducer(contentProducer); + try (AutoLock lock = contentProducer.lock()) { contentProducer.setInterceptor(new GzipHttpInputInterceptor(inflaterPool, new ArrayByteBufferPool(1, 1, 2), 32)); @@ -142,10 +142,11 @@ public void testBlockingContentProducerGzipInterceptorWithTinyBuffers() buffers[1] = gzipByteBuffer(uncompressedBuffers[1]); buffers[2] = gzipByteBuffer(uncompressedBuffers[2]); - AtomicReference ref = new AtomicReference<>(); - ArrayDelayedHttpChannel httpChannel = new ArrayDelayedHttpChannel(buffers, new HttpInput.EofContent(), scheduledExecutorService, () -> ref.get().onContentProducible()); + ContentListener contentListener = new ContentListener(); + ArrayDelayedHttpChannel httpChannel = new ArrayDelayedHttpChannel(buffers, new HttpInput.EofContent(), scheduledExecutorService, contentListener); ContentProducer contentProducer = new BlockingContentProducer(new AsyncContentProducer(httpChannel)); - ref.set(contentProducer); + contentListener.setContentProducer(contentProducer); + try (AutoLock lock = contentProducer.lock()) { contentProducer.setInterceptor(new GzipHttpInputInterceptor(inflaterPool, new ArrayByteBufferPool(1, 1, 2), 1)); @@ -171,10 +172,11 @@ public void testBlockingContentProducerGzipInterceptorWithError() buffers[1] = gzipByteBuffer(uncompressedBuffers[1]); buffers[2] = gzipByteBuffer(uncompressedBuffers[2]); - AtomicReference ref = new AtomicReference<>(); - ArrayDelayedHttpChannel httpChannel = new ArrayDelayedHttpChannel(buffers, new HttpInput.ErrorContent(expectedError), scheduledExecutorService, () -> ref.get().onContentProducible()); + ContentListener contentListener = new ContentListener(); + ArrayDelayedHttpChannel httpChannel = new ArrayDelayedHttpChannel(buffers, new HttpInput.ErrorContent(expectedError), scheduledExecutorService, contentListener); ContentProducer contentProducer = new BlockingContentProducer(new AsyncContentProducer(httpChannel)); - ref.set(contentProducer); + contentListener.setContentProducer(contentProducer); + try (AutoLock lock = contentProducer.lock()) { contentProducer.setInterceptor(new GzipHttpInputInterceptor(inflaterPool, new ArrayByteBufferPool(1, 1, 2), 32)); @@ -257,9 +259,26 @@ private static ByteBuffer gzipByteBuffer(ByteBuffer uncompressedBuffer) } } - private interface ContentListener + private static class ContentListener { - void onContent(); + private ContentProducer contentProducer; + + private ContentListener() + { + } + + private void onContent() + { + try (AutoLock lock = contentProducer.lock()) + { + contentProducer.onContentProducible(); + } + } + + private void setContentProducer(ContentProducer contentProducer) + { + this.contentProducer = contentProducer; + } } private static class ArrayDelayedHttpChannel extends HttpChannel