From 254f80c66129efe449457314523925913b8f1882 Mon Sep 17 00:00:00 2001 From: David Matejcek Date: Wed, 18 Nov 2020 12:24:46 +0100 Subject: [PATCH 01/26] Issue #2016 Possibility to run tests with different JSSE implementations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - default is none (uses JDK's impl) - profile npn uses grizzly-npn-bootstrap - profile openjsse uses openjsse - versions can be overriden from command line (to compare results) Signed-off-by: David Matějček --- modules/bundles/http/pom.xml | 3 +-- modules/http2/pom.xml | 44 +++++++++++++++++++++++++++++++----- pom.xml | 14 +++++++----- 3 files changed, 47 insertions(+), 14 deletions(-) diff --git a/modules/bundles/http/pom.xml b/modules/bundles/http/pom.xml index 50aefba1f8..2cfacf86df 100755 --- a/modules/bundles/http/pom.xml +++ b/modules/bundles/http/pom.xml @@ -52,14 +52,13 @@ org.glassfish.grizzly grizzly-npn-api - ${grizzly.alpn.version} + ${grizzly.npn.api.version} org.glassfish.grizzly grizzly-http-server-multipart - install diff --git a/modules/http2/pom.xml b/modules/http2/pom.xml index bba200786c..8282bfd511 100644 --- a/modules/http2/pom.xml +++ b/modules/http2/pom.xml @@ -30,6 +30,9 @@ grizzly-http2 grizzly-http2 + + + @@ -39,7 +42,7 @@ org.glassfish.grizzly grizzly-http-server - provided true @@ -47,10 +50,9 @@ org.glassfish.grizzly grizzly-npn-api - ${grizzly.alpn.version} + ${grizzly.npn.api.version} provided - org.mockito mockito-core @@ -107,9 +109,7 @@ maven-surefire-plugin - - -XX:+HeapDumpOnOutOfMemoryError - + -XX:+HeapDumpOnOutOfMemoryError ${bootClasspath} @@ -126,4 +126,36 @@ + + + + + openjsse + + -Xbootclasspath/p:${settings.localRepository}/org/openjsse/openjsse/${openjsse.version}/openjsse-${openjsse.version}.jar + + + + org.openjsse + openjsse + ${openjsse.version} + provided + + + + + npn + + -Xbootclasspath/p:${settings.localRepository}/org/glassfish/grizzly/grizzly-npn-bootstrap/${grizzly.npn.bootstrap.version}/grizzly-npn-bootstrap-${grizzly.npn.bootstrap.version}.jar + + + + org.glassfish.grizzly + grizzly-npn-api + ${grizzly.npn.bootstrap.version} + provided + + + + diff --git a/pom.xml b/pom.xml index 457a88e052..f651bef7ff 100644 --- a/pom.xml +++ b/pom.xml @@ -125,8 +125,10 @@ 4.2.1 2.4 4.0.0 + 2.0 + 2.0 + 1.1.5 4.2.0 - 2.0 @@ -218,7 +220,7 @@ - + true @@ -230,7 +232,7 @@ -Xlint:unchecked,deprecation,fallthrough,finally,cast,dep-ann,empty,overrides - + true org.apache.maven.plugins maven-jar-plugin @@ -246,7 +248,7 @@ - + true org.apache.maven.plugins maven-source-plugin @@ -259,7 +261,7 @@ - + org.apache.maven.plugins @@ -271,7 +273,7 @@ ${project.name} ${project.version} - + org.glassfish.copyright glassfish-copyright-maven-plugin From 1775e355193a1ad5b79457bdaf9df9848dbdde7a Mon Sep 17 00:00:00 2001 From: David Matejcek Date: Wed, 18 Nov 2020 12:50:46 +0100 Subject: [PATCH 02/26] Issue #2016 Logging in tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - important loggers are explicitly named in logging.properties Signed-off-by: David Matějček --- .../glassfish/grizzly/http2/AbstractHttp2Test.java | 11 +++++++++++ modules/http2/src/test/resources/logging.properties | 13 +++++++++++++ 2 files changed, 24 insertions(+) create mode 100644 modules/http2/src/test/resources/logging.properties diff --git a/modules/http2/src/test/java/org/glassfish/grizzly/http2/AbstractHttp2Test.java b/modules/http2/src/test/java/org/glassfish/grizzly/http2/AbstractHttp2Test.java index 4d5a951666..dcbb038d8f 100644 --- a/modules/http2/src/test/java/org/glassfish/grizzly/http2/AbstractHttp2Test.java +++ b/modules/http2/src/test/java/org/glassfish/grizzly/http2/AbstractHttp2Test.java @@ -16,10 +16,12 @@ package org.glassfish.grizzly.http2; +import java.io.IOException; import java.io.UnsupportedEncodingException; import java.net.URL; import java.util.Arrays; import java.util.Collection; +import java.util.logging.LogManager; import java.util.logging.Logger; import org.glassfish.grizzly.Buffer; @@ -49,6 +51,15 @@ * @author Alexey Stashok */ public abstract class AbstractHttp2Test { + static { + try { + LogManager.getLogManager().readConfiguration(AbstractHttp2Test.class.getResourceAsStream("/logging.properties")); + } catch (SecurityException | IOException e) { + e.printStackTrace(); + } + } + + protected static final Logger LOGGER = Grizzly.logger(AbstractHttp2Test.class); private volatile static SSLEngineConfigurator clientSSLEngineConfigurator; diff --git a/modules/http2/src/test/resources/logging.properties b/modules/http2/src/test/resources/logging.properties new file mode 100644 index 0000000000..c280aa86dd --- /dev/null +++ b/modules/http2/src/test/resources/logging.properties @@ -0,0 +1,13 @@ +handlers=java.util.logging.ConsoleHandler + +java.util.logging.ConsoleHandler.encoding=UTF-8 +java.util.logging.ConsoleHandler.level=ALL + +# Note: NIOOutputSinksTest fails with too verbose logging + +.level=INFO +org.glassfish.grizzly.http2.level=INFO +org.glassfish.grizzly.filterchain.DefaultFilterChain.level=INFO +org.glassfish.grizzly.http2.DefaultInputBuffer.level=INFO +org.glassfish.grizzly.http2.Http2Stream.level=INFO +org.glassfish.grizzly.http2.NetLogger.level=FINE From f4866af90e94c12d14d0a3b5817af6f7a23b4087 Mon Sep 17 00:00:00 2001 From: David Matejcek Date: Wed, 2 Sep 2020 15:24:39 +0200 Subject: [PATCH 03/26] Issue #2016 Fixed dependency artifactId MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: David Matějček --- modules/http2/pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/http2/pom.xml b/modules/http2/pom.xml index 8282bfd511..1ba1eb4c6a 100644 --- a/modules/http2/pom.xml +++ b/modules/http2/pom.xml @@ -151,7 +151,7 @@ org.glassfish.grizzly - grizzly-npn-api + grizzly-npn-bootstrap ${grizzly.npn.bootstrap.version} provided From a2f7f4cdd45938ee65db186aebfb528139c3cd66 Mon Sep 17 00:00:00 2001 From: David Matejcek Date: Wed, 18 Nov 2020 13:19:08 +0100 Subject: [PATCH 04/26] Issue #2016 Add support for JDK11 ALPN MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Create new phase in the handshake listener to allow registration of the custom ALPN logic. This allows the HTTP/2 filter to work correctly. Signed-off-by: Matthew Gill Signed-off-by: David Matějček --- .../grizzly/ssl/HandshakeListener.java | 12 ++++++++++++ .../glassfish/grizzly/ssl/SSLBaseFilter.java | 12 ++---------- .../glassfish/grizzly/http2/AlpnSupport.java | 17 ++++++++--------- 3 files changed, 22 insertions(+), 19 deletions(-) create mode 100644 modules/grizzly/src/main/java/org/glassfish/grizzly/ssl/HandshakeListener.java diff --git a/modules/grizzly/src/main/java/org/glassfish/grizzly/ssl/HandshakeListener.java b/modules/grizzly/src/main/java/org/glassfish/grizzly/ssl/HandshakeListener.java new file mode 100644 index 0000000000..6c6f4870cf --- /dev/null +++ b/modules/grizzly/src/main/java/org/glassfish/grizzly/ssl/HandshakeListener.java @@ -0,0 +1,12 @@ +package org.glassfish.grizzly.ssl; + +import javax.net.ssl.SSLEngine; + +import org.glassfish.grizzly.Connection; + +public interface HandshakeListener { + void onInit(Connection connection, SSLEngine sslEngine); + void onStart(Connection connection); + void onComplete(Connection connection); + void onFailure(Connection connection, Throwable t); +} diff --git a/modules/grizzly/src/main/java/org/glassfish/grizzly/ssl/SSLBaseFilter.java b/modules/grizzly/src/main/java/org/glassfish/grizzly/ssl/SSLBaseFilter.java index c8c76fc2a6..272483989a 100644 --- a/modules/grizzly/src/main/java/org/glassfish/grizzly/ssl/SSLBaseFilter.java +++ b/modules/grizzly/src/main/java/org/glassfish/grizzly/ssl/SSLBaseFilter.java @@ -13,7 +13,7 @@ * https://www.gnu.org/software/classpath/license.html. * * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 - * + * * Contributors: * Payara Services - Add support for JDK 9 ALPN API * - Propagate stop action on a closed SSL connection @@ -193,7 +193,7 @@ public long getHandshakeTimeout(final TimeUnit timeUnit) { /** * Sets the handshake timeout. - * + * * @param handshakeTimeout timeout value, or -1 means for non-blocking handshake mode. * @param timeUnit {@link TimeUnit} */ @@ -892,14 +892,6 @@ public void onComplete(final Context context, Object data) throws IOException { } // END InternalProcessingHandler - public interface HandshakeListener { - void onStart(Connection connection); - - void onComplete(Connection connection); - - void onFailure(Connection connection, Throwable t); - } - protected static class SSLTransportFilterWrapper extends TransportFilter { protected final TransportFilter wrappedFilter; protected final SSLBaseFilter sslBaseFilter; diff --git a/modules/http2/src/main/java/org/glassfish/grizzly/http2/AlpnSupport.java b/modules/http2/src/main/java/org/glassfish/grizzly/http2/AlpnSupport.java index f41b5d1a70..4f5d16714d 100644 --- a/modules/http2/src/main/java/org/glassfish/grizzly/http2/AlpnSupport.java +++ b/modules/http2/src/main/java/org/glassfish/grizzly/http2/AlpnSupport.java @@ -13,7 +13,7 @@ * https://www.gnu.org/software/classpath/license.html. * * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 - * + * * Contributors: * Payara Services - Add support for JDK 9 ALPN API */ @@ -56,7 +56,7 @@ public class AlpnSupport { private static final AlpnSupport INSTANCE; private static final Method nativeHandshakeMethod; - + static { boolean isExtensionFound = false; Method setHandshakeAlpnSelector = null; @@ -109,8 +109,7 @@ private static void setConnection(final SSLEngine engine, final Connection co private final Map clientSideNegotiators = new WeakHashMap<>(); private final ReadWriteLock clientSideLock = new ReentrantReadWriteLock(); - private final HandshakeListener handshakeListener = - new HandshakeListener() { + private final HandshakeListener handshakeListener = new HandshakeListener() { @Override public void onInit(final Connection connection, final SSLEngine sslEngine) { @@ -136,7 +135,7 @@ public void onStart(final Connection connection) { if (sslEngine.getUseClientMode()) { AlpnClientNegotiator negotiator = getClientNegotiator(connection); - + if (negotiator != null) { // add a CloseListener to ensure we remove the // negotiator associated with this SSLEngine @@ -152,7 +151,7 @@ public void onClosed(Closeable closeable, CloseType type) throws IOException { } } else { AlpnServerNegotiator negotiator = getServerNegotiator(connection); - + if (negotiator != null) { // add a CloseListener to ensure we remove the @@ -180,6 +179,7 @@ public void onFailure(Connection connection, Throwable t) { } }; + private AlpnSupport() { } @@ -222,12 +222,11 @@ private void putClientSideNegotiator(final Object object, final AlpnClientNegoti clientSideLock.writeLock().unlock(); } } - private AlpnClientNegotiator getClientNegotiator(Connection connection) { AlpnClientNegotiator negotiator; clientSideLock.readLock().lock(); - + try { negotiator = clientSideNegotiators.get(connection); if (negotiator == null) { @@ -243,7 +242,7 @@ private AlpnClientNegotiator getClientNegotiator(Connection connection) { private AlpnServerNegotiator getServerNegotiator(Connection connection) { AlpnServerNegotiator negotiator; serverSideLock.readLock().lock(); - + try { negotiator = serverSideNegotiators.get(connection); if (negotiator == null) { From 627b53f7766f2b05f4c7dfbd32121dbaf9f9e7bc Mon Sep 17 00:00:00 2001 From: David Matejcek Date: Wed, 18 Nov 2020 13:23:10 +0100 Subject: [PATCH 05/26] Issue #2016 Prevent HTTP/2 push when globally disabled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Streams still allowed pushing resources when globally disabled as the stream itself and the session both allow it. This change makes the stream also respect the global configuration. Signed-off-by: Matthew Gill Signed-off-by: David Matějček --- .../main/java/org/glassfish/grizzly/http2/Http2Session.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2Session.java b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2Session.java index f26f82f2ba..01ef91b00f 100644 --- a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2Session.java +++ b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2Session.java @@ -1,5 +1,6 @@ /* - * Copyright (c) 2012, 2020 Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2012, 2020 Oracle and/or its affiliates and others. + * All rights reserved. * * This program and the accompanying materials are made available under the * terms of the Eclipse Public License v. 2.0, which is available at @@ -489,7 +490,7 @@ void setPeerMaxConcurrentStreams(int peerMaxConcurrentStreams) { * Push is enabled by default. */ public boolean isPushEnabled() { - return pushEnabled; + return pushEnabled && http2Configuration.isPushEnabled(); } /** From 7254635b92d480272d71057bc20b40ea975755e6 Mon Sep 17 00:00:00 2001 From: David Matejcek Date: Wed, 18 Nov 2020 13:25:45 +0100 Subject: [PATCH 06/26] Issue #2016 Fixed Tyrus dependency on HandshakeListener interface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - moving this interface out is a breaking change Signed-off-by: David Matějček --- .../org/glassfish/grizzly/ssl/HandshakeListener.java | 12 ------------ .../org/glassfish/grizzly/ssl/SSLBaseFilter.java | 11 +++++++++++ .../org/glassfish/grizzly/http2/AlpnSupport.java | 2 +- 3 files changed, 12 insertions(+), 13 deletions(-) delete mode 100644 modules/grizzly/src/main/java/org/glassfish/grizzly/ssl/HandshakeListener.java diff --git a/modules/grizzly/src/main/java/org/glassfish/grizzly/ssl/HandshakeListener.java b/modules/grizzly/src/main/java/org/glassfish/grizzly/ssl/HandshakeListener.java deleted file mode 100644 index 6c6f4870cf..0000000000 --- a/modules/grizzly/src/main/java/org/glassfish/grizzly/ssl/HandshakeListener.java +++ /dev/null @@ -1,12 +0,0 @@ -package org.glassfish.grizzly.ssl; - -import javax.net.ssl.SSLEngine; - -import org.glassfish.grizzly.Connection; - -public interface HandshakeListener { - void onInit(Connection connection, SSLEngine sslEngine); - void onStart(Connection connection); - void onComplete(Connection connection); - void onFailure(Connection connection, Throwable t); -} diff --git a/modules/grizzly/src/main/java/org/glassfish/grizzly/ssl/SSLBaseFilter.java b/modules/grizzly/src/main/java/org/glassfish/grizzly/ssl/SSLBaseFilter.java index 272483989a..2da342a7b5 100644 --- a/modules/grizzly/src/main/java/org/glassfish/grizzly/ssl/SSLBaseFilter.java +++ b/modules/grizzly/src/main/java/org/glassfish/grizzly/ssl/SSLBaseFilter.java @@ -1001,4 +1001,15 @@ public Buffer clone(final Connection connection, final Buffer originalMessage) { return originalMessage; } } + + + // don't move to own file, Tyrus has a dependency on this interface + public interface HandshakeListener { + default void onInit(Connection connection, SSLEngine sslEngine) { + // nothing + } + void onStart(Connection connection); + void onComplete(Connection connection); + void onFailure(Connection connection, Throwable t); + } } diff --git a/modules/http2/src/main/java/org/glassfish/grizzly/http2/AlpnSupport.java b/modules/http2/src/main/java/org/glassfish/grizzly/http2/AlpnSupport.java index 4f5d16714d..275d6efd09 100644 --- a/modules/http2/src/main/java/org/glassfish/grizzly/http2/AlpnSupport.java +++ b/modules/http2/src/main/java/org/glassfish/grizzly/http2/AlpnSupport.java @@ -41,8 +41,8 @@ import org.glassfish.grizzly.npn.AlpnClientNegotiator; import org.glassfish.grizzly.npn.AlpnServerNegotiator; import org.glassfish.grizzly.npn.NegotiationSupport; -import org.glassfish.grizzly.ssl.HandshakeListener; import org.glassfish.grizzly.ssl.SSLBaseFilter; +import org.glassfish.grizzly.ssl.SSLBaseFilter.HandshakeListener; import org.glassfish.grizzly.ssl.SSLUtils; /** From 7688da134c0912105050c9db48901c859552df7b Mon Sep 17 00:00:00 2001 From: David Matejcek Date: Wed, 18 Nov 2020 14:59:42 +0100 Subject: [PATCH 07/26] Issue #2016 Added trailer to output queue to prevent early connection closure. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Matt Gill Signed-off-by: David Matějček --- .../grizzly/http2/DefaultOutputSink.java | 95 +++++++++++++++---- 1 file changed, 75 insertions(+), 20 deletions(-) diff --git a/modules/http2/src/main/java/org/glassfish/grizzly/http2/DefaultOutputSink.java b/modules/http2/src/main/java/org/glassfish/grizzly/http2/DefaultOutputSink.java index 09b402dfba..cbce6b2a02 100644 --- a/modules/http2/src/main/java/org/glassfish/grizzly/http2/DefaultOutputSink.java +++ b/modules/http2/src/main/java/org/glassfish/grizzly/http2/DefaultOutputSink.java @@ -17,6 +17,8 @@ package org.glassfish.grizzly.http2; import static org.glassfish.grizzly.http2.Termination.OUT_FIN_TERMINATION; +import static java.util.logging.Level.FINE; +import static java.util.logging.Level.WARNING; import java.io.IOException; import java.util.ArrayList; @@ -24,7 +26,7 @@ import java.util.List; import java.util.Map; import java.util.concurrent.atomic.AtomicInteger; -import java.util.logging.Level; +import java.util.concurrent.locks.ReentrantLock; import java.util.logging.Logger; import org.glassfish.grizzly.Buffer; @@ -113,8 +115,8 @@ public void notifyWritePossible(final WriteHandler writeHandler) { private void assertReady() throws IOException { // if the last frame (fin flag == 1) has been queued already - throw an IOException if (isTerminated()) { - if (LOGGER.isLoggable(Level.FINE)) { - LOGGER.log(Level.FINE, "Terminated!!! id={0} description={1}", new Object[] { stream.getId(), terminationFlag.getDescription() }); + if (LOGGER.isLoggable(FINE)) { + LOGGER.log(FINE, "Terminated!!! id={0} description={1}", new Object[] { stream.getId(), terminationFlag.getDescription() }); } throw new IOException(terminationFlag.getDescription()); } else if (isLastFrameQueued) { @@ -162,6 +164,18 @@ public void onPeerWindowUpdate(final int delta) throws Http2StreamException { final boolean isZeroSizeData = outputQueueRecord.isZeroSizeData; final Source resource = outputQueueRecord.resource; + final HttpTrailer currentTrailer = outputQueueRecord.trailer; + final MessageCloner messageCloner = outputQueueRecord.cloner; + + if (currentTrailer != null) { + try { + sendTrailers(completionHandler, messageCloner, currentTrailer); + } catch (IOException ex) { + LOGGER.log(WARNING, "Error sending trailers.", ex); + } + return; + } + // check if output record's buffer is fitting into window size // if not - split it into 2 parts: part to send, part to keep in the queue final int bytesToSend = checkOutputWindow(resource.remaining()); @@ -205,6 +219,21 @@ public void onPeerWindowUpdate(final int delta) throws Http2StreamException { break; } } + + if (outputQueue.peek() != null && outputQueue.peek().trailer != null) { + // pick up the first output record in the queue + final OutputQueueRecord outputQueueRecord = outputQueue.poll(); + + final FlushCompletionHandler completionHandler = outputQueueRecord.chunkedCompletionHandler; + final HttpTrailer currentTrailer = outputQueueRecord.trailer; + final MessageCloner messageCloner = outputQueueRecord.cloner; + try { + sendTrailers(completionHandler, messageCloner, currentTrailer); + } catch (IOException ex) { + LOGGER.log(WARNING, "Error sending trailers.", ex); + } + return; + } } /** @@ -232,7 +261,7 @@ public synchronized void writeDownStream(final HttpPacket httpPacket, final Filt List headerFrames = null; OutputQueueRecord outputQueueRecord = null; - boolean isDeflaterLocked = false; + final ReentrantLock deflatorLock = http2Session.getDeflaterLock(); try { // try-finally block to release deflater lock if needed @@ -243,8 +272,7 @@ public synchronized void writeDownStream(final HttpPacket httpPacket, final Filt || httpContent != null && httpContent.isLast() && !httpContent.getContent().hasRemaining(); // !!!!! LOCK the deflater - isDeflaterLocked = true; - http2Session.getDeflaterLock().lock(); + deflatorLock.lock(); final boolean logging = NetLogger.isActive(); final Map capture = logging ? new HashMap<>() : null; headerFrames = http2Session.encodeHttpHeaderAsHeaderFrames(ctx, httpHeader, stream.getId(), isNoPayload, null, capture); @@ -294,15 +322,6 @@ public synchronized void writeDownStream(final HttpPacket httpPacket, final Filt Buffer data = httpContent.getContent(); final int dataSize = data.remaining(); - if (isLast && dataSize == 0) { - if (isTrailer) { - // !!!!! LOCK the deflater - isDeflaterLocked = true; - sendTrailers(completionHandler, messageCloner, (HttpTrailer) httpContent); - } - close(); - return; - } unflushedWritesCounter.incrementAndGet(); final FlushCompletionHandler flushCompletionHandler = new FlushCompletionHandler(completionHandler); @@ -322,7 +341,17 @@ public synchronized void writeDownStream(final HttpPacket httpPacket, final Filt isDataCloned = true; } - outputQueueRecord = new OutputQueueRecord(Source.factory(stream).createBufferSource(data), flushCompletionHandler, isLast, isZeroSizeData); + if (isTrailer) { + outputQueueRecord = new OutputQueueRecord( + Source.factory(stream).createBufferSource(data), + flushCompletionHandler, + (HttpTrailer) httpContent, + isZeroSizeData); + } else { + outputQueueRecord = new OutputQueueRecord( + Source.factory(stream).createBufferSource(data), + flushCompletionHandler, isLast, isZeroSizeData); + } outputQueue.offer(outputQueueRecord); @@ -336,7 +365,6 @@ public synchronized void writeDownStream(final HttpPacket httpPacket, final Filt } // our element is first in the output queue - final int remaining = data.remaining(); // check if output record's buffer is fitting into window size @@ -388,7 +416,6 @@ public synchronized void writeDownStream(final HttpPacket httpPacket, final Filt if (isLast) { if (isTrailer) { // !!!!! LOCK the deflater - isDeflaterLocked = true; sendTrailers(completionHandler, messageCloner, (HttpTrailer) httpContent); } close(); @@ -396,8 +423,8 @@ public synchronized void writeDownStream(final HttpPacket httpPacket, final Filt } } finally { - if (isDeflaterLocked) { - http2Session.getDeflaterLock().unlock(); + if (deflatorLock.isHeldByCurrentThread()) { + deflatorLock.unlock(); } } @@ -559,6 +586,17 @@ private void addOutputQueueRecord(OutputQueueRecord outputQueueRecord) throws Ht final FlushCompletionHandler chunkedCompletionHandler = outputQueueRecord.chunkedCompletionHandler; + final HttpTrailer currentTrailer = outputQueueRecord.trailer; + final MessageCloner messageCloner = outputQueueRecord.cloner; + if (currentTrailer != null) { + try { + sendTrailers(chunkedCompletionHandler, messageCloner, currentTrailer); + } catch (IOException ex) { + LOGGER.log(WARNING, "Error sending trailers.", ex); + } + return; + } + boolean isLast = outputQueueRecord.isLast; final boolean isZeroSizeData = outputQueueRecord.isZeroSizeData; @@ -636,12 +674,17 @@ private void sendTrailers(final CompletionHandler completionHandler } unflushedWritesCounter.incrementAndGet(); flushToConnectionOutputSink(trailerFrames, null, new FlushCompletionHandler(completionHandler), messageCloner, true); + http2Session.getDeflaterLock().unlock(); + close(); } private static class OutputQueueRecord extends AsyncQueueRecord { private Source resource; private FlushCompletionHandler chunkedCompletionHandler; + private HttpTrailer trailer; + private MessageCloner cloner; + private boolean isLast; private final boolean isZeroSizeData; @@ -655,6 +698,18 @@ public OutputQueueRecord(final Source resource, final FlushCompletionHandler com this.isZeroSizeData = isZeroSizeData; } + public OutputQueueRecord(final Source resource, + final FlushCompletionHandler completionHandler, + final HttpTrailer trailer, final boolean isZeroDataSize) { + super(null, null, null); + + this.resource = resource; + this.chunkedCompletionHandler = completionHandler; + this.isLast = true; + this.trailer = trailer; + this.isZeroSizeData = isZeroDataSize; + } + private void incChunksCounter() { if (chunkedCompletionHandler != null) { chunkedCompletionHandler.incChunks(); From bfbf47147670f8de57c93b33603472fe9e3f8819 Mon Sep 17 00:00:00 2001 From: Matt Gill Date: Mon, 20 Aug 2018 12:55:12 +0100 Subject: [PATCH 08/26] Issue #2016 Removed second check for trailer. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Matt Gill Signed-off-by: David Matějček --- .../grizzly/http2/DefaultOutputSink.java | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/modules/http2/src/main/java/org/glassfish/grizzly/http2/DefaultOutputSink.java b/modules/http2/src/main/java/org/glassfish/grizzly/http2/DefaultOutputSink.java index cbce6b2a02..f303167b84 100644 --- a/modules/http2/src/main/java/org/glassfish/grizzly/http2/DefaultOutputSink.java +++ b/modules/http2/src/main/java/org/glassfish/grizzly/http2/DefaultOutputSink.java @@ -141,7 +141,8 @@ public void onPeerWindowUpdate(final int delta) throws Http2StreamException { availStreamWindowSize.addAndGet(delta); // try to write until window limit allows - while (isWantToWrite() && !outputQueue.isEmpty()) { + while ((isWantToWrite() && !outputQueue.isEmpty()) + || (outputQueue.peek() != null && outputQueue.peek().trailer != null)) { // pick up the first output record in the queue OutputQueueRecord outputQueueRecord = outputQueue.poll(); @@ -169,6 +170,7 @@ public void onPeerWindowUpdate(final int delta) throws Http2StreamException { if (currentTrailer != null) { try { + outputQueueRecord = null; sendTrailers(completionHandler, messageCloner, currentTrailer); } catch (IOException ex) { LOGGER.log(WARNING, "Error sending trailers.", ex); @@ -221,18 +223,7 @@ public void onPeerWindowUpdate(final int delta) throws Http2StreamException { } if (outputQueue.peek() != null && outputQueue.peek().trailer != null) { - // pick up the first output record in the queue - final OutputQueueRecord outputQueueRecord = outputQueue.poll(); - - final FlushCompletionHandler completionHandler = outputQueueRecord.chunkedCompletionHandler; - final HttpTrailer currentTrailer = outputQueueRecord.trailer; - final MessageCloner messageCloner = outputQueueRecord.cloner; - try { - sendTrailers(completionHandler, messageCloner, currentTrailer); - } catch (IOException ex) { - LOGGER.log(WARNING, "Error sending trailers.", ex); - } - return; + LOGGER.warning("Trailer frame is going to get ignored."); } } @@ -346,7 +337,7 @@ public synchronized void writeDownStream(final HttpPacket httpPacket, final Filt Source.factory(stream).createBufferSource(data), flushCompletionHandler, (HttpTrailer) httpContent, - isZeroSizeData); + false); } else { outputQueueRecord = new OutputQueueRecord( Source.factory(stream).createBufferSource(data), From 57c951f0da90ea6bda172a3abd7e1c09d1e92ba4 Mon Sep 17 00:00:00 2001 From: Matt Gill Date: Mon, 20 Aug 2018 13:30:35 +0100 Subject: [PATCH 09/26] Issue #2016 Removed second deflator lock. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Matt Gill Signed-off-by: David Matějček --- .../org/glassfish/grizzly/http2/DefaultOutputSink.java | 7 ------- 1 file changed, 7 deletions(-) diff --git a/modules/http2/src/main/java/org/glassfish/grizzly/http2/DefaultOutputSink.java b/modules/http2/src/main/java/org/glassfish/grizzly/http2/DefaultOutputSink.java index f303167b84..b49cf30a7e 100644 --- a/modules/http2/src/main/java/org/glassfish/grizzly/http2/DefaultOutputSink.java +++ b/modules/http2/src/main/java/org/glassfish/grizzly/http2/DefaultOutputSink.java @@ -221,10 +221,6 @@ public void onPeerWindowUpdate(final int delta) throws Http2StreamException { break; } } - - if (outputQueue.peek() != null && outputQueue.peek().trailer != null) { - LOGGER.warning("Trailer frame is going to get ignored."); - } } /** @@ -409,7 +405,6 @@ public synchronized void writeDownStream(final HttpPacket httpPacket, final Filt // !!!!! LOCK the deflater sendTrailers(completionHandler, messageCloner, (HttpTrailer) httpContent); } - close(); return; } @@ -651,7 +646,6 @@ private void releaseWriteQueueSpace(final int justSentBytes, final boolean isAto private void sendTrailers(final CompletionHandler completionHandler, final MessageCloner messageCloner, final HttpTrailer httpContent) throws IOException { - http2Session.getDeflaterLock().lock(); final boolean logging = NetLogger.isActive(); final Map capture = logging ? new HashMap<>() : null; List trailerFrames = http2Session.encodeTrailersAsHeaderFrames(stream.getId(), new ArrayList<>(4), httpContent.getHeaders(), capture); @@ -665,7 +659,6 @@ private void sendTrailers(final CompletionHandler completionHandler } unflushedWritesCounter.incrementAndGet(); flushToConnectionOutputSink(trailerFrames, null, new FlushCompletionHandler(completionHandler), messageCloner, true); - http2Session.getDeflaterLock().unlock(); close(); } From a90d6e9d63f58efbb1ecc8ddab456836eaaac04d Mon Sep 17 00:00:00 2001 From: David Matejcek Date: Fri, 4 Sep 2020 17:11:52 +0200 Subject: [PATCH 10/26] Issue #2016 Fixed TrailersTest - missing lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - partial cleanup, but requires much more than this - DefaultOutputSink - combination of "fixes by refactoring" mixed with the changes in the flow of the logic - sendTrailers now locks the deflater - added some javadocs and much more logs Signed-off-by: David Matějček --- .../filterchain/DefaultFilterChain.java | 13 +- .../glassfish/grizzly/ssl/SSLBaseFilter.java | 4 +- .../glassfish/grizzly/http/HttpContent.java | 7 +- .../glassfish/grizzly/http/HttpTrailer.java | 2 +- .../grizzly/http2/DefaultOutputSink.java | 267 ++++++++++-------- .../glassfish/grizzly/http2/Http2AddOn.java | 24 +- .../grizzly/http2/Http2ClientFilter.java | 53 ++-- .../grizzly/http2/Http2ServerFilter.java | 46 +-- .../glassfish/grizzly/http2/Http2Session.java | 5 +- .../http2/utils/ChunkedCompletionHandler.java | 19 +- .../glassfish/grizzly/http2/TrailersTest.java | 4 +- .../src/test/resources/logging.properties | 6 +- 12 files changed, 250 insertions(+), 200 deletions(-) diff --git a/modules/grizzly/src/main/java/org/glassfish/grizzly/filterchain/DefaultFilterChain.java b/modules/grizzly/src/main/java/org/glassfish/grizzly/filterchain/DefaultFilterChain.java index 3c11239bfe..0412825230 100644 --- a/modules/grizzly/src/main/java/org/glassfish/grizzly/filterchain/DefaultFilterChain.java +++ b/modules/grizzly/src/main/java/org/glassfish/grizzly/filterchain/DefaultFilterChain.java @@ -59,9 +59,6 @@ public final class DefaultFilterChain extends ListFacadeFilterChain { private final FiltersStateFactory filtersStateFactory = new FiltersStateFactory(); - /** - * Logger - */ private static final Logger LOGGER = Grizzly.logger(DefaultFilterChain.class); public DefaultFilterChain() { @@ -101,7 +98,7 @@ public ProcessorResult process(final Context context) { /** * Execute this FilterChain. - * + * * @param ctx {@link FilterChainContext} processing context */ @Override @@ -242,13 +239,15 @@ protected NextAction executeFilter(final FilterExecutor executor, final Filter c NextAction nextNextAction; do { if (LOGGER.isLoggable(Level.FINEST)) { - LOGGER.log(Level.FINE, "Execute filter. filter={0} context={1}", new Object[] { currentFilter, ctx }); + LOGGER.log(Level.FINE, "before filter execution. filter={0} context={1}", + new Object[]{currentFilter, ctx}); } // execute the task nextNextAction = executor.execute(currentFilter, ctx); if (LOGGER.isLoggable(Level.FINEST)) { - LOGGER.log(Level.FINE, "after execute filter. filter={0} context={1} nextAction={2}", new Object[] { currentFilter, ctx, nextNextAction }); + LOGGER.log(Level.FINE, "after execute filter. filter={0} context={1} nextAction={2}", + new Object[] { currentFilter, ctx, nextNextAction }); } } while (nextNextAction.type() == RerunFilterAction.TYPE); @@ -402,7 +401,7 @@ public void fail(FilterChainContext context, Throwable failure) { /** * Notify the filters about error. - * + * * @param ctx {@link FilterChainContext} * @return position of the last executed {@link Filter} */ diff --git a/modules/grizzly/src/main/java/org/glassfish/grizzly/ssl/SSLBaseFilter.java b/modules/grizzly/src/main/java/org/glassfish/grizzly/ssl/SSLBaseFilter.java index 2da342a7b5..e22c0e00a2 100644 --- a/modules/grizzly/src/main/java/org/glassfish/grizzly/ssl/SSLBaseFilter.java +++ b/modules/grizzly/src/main/java/org/glassfish/grizzly/ssl/SSLBaseFilter.java @@ -794,9 +794,7 @@ private static Certificate[] getPeerCertificates(final SSLConnectionContext sslC try { return sslCtx.getSslEngine().getSession().getPeerCertificates(); } catch (Throwable t) { - if (LOGGER.isLoggable(Level.FINE)) { - LOGGER.log(Level.FINE, "Error getting client certs", t); - } + LOGGER.log(Level.FINE, "Error getting client certs", t); return null; } } diff --git a/modules/http/src/main/java/org/glassfish/grizzly/http/HttpContent.java b/modules/http/src/main/java/org/glassfish/grizzly/http/HttpContent.java index 7e5dfaed46..2ecb48f856 100644 --- a/modules/http/src/main/java/org/glassfish/grizzly/http/HttpContent.java +++ b/modules/http/src/main/java/org/glassfish/grizzly/http/HttpContent.java @@ -70,7 +70,6 @@ public static HttpContent create(final HttpHeader httpHeader, final boolean isLa public static HttpContent create(final HttpHeader httpHeader, final boolean isLast, Buffer content) { content = content != null ? content : Buffers.EMPTY_BUFFER; - final HttpContent httpContent = ThreadCache.takeFromCache(CACHE_IDX); if (httpContent != null) { httpContent.httpHeader = httpHeader; @@ -137,10 +136,8 @@ public final HttpHeader getHttpHeader() { } /** - * Return true, if the current content chunk is last, or false, if there are content chunks to follow. - * - * @return true, if the current content chunk is last, or false, if there are content chunks to - * follow. + * @return true, if the current content chunk is last, + * or false, if there are content chunks to follow. */ public boolean isLast() { return isLast; diff --git a/modules/http/src/main/java/org/glassfish/grizzly/http/HttpTrailer.java b/modules/http/src/main/java/org/glassfish/grizzly/http/HttpTrailer.java index d35aa5ea1f..7ba6e9d25b 100644 --- a/modules/http/src/main/java/org/glassfish/grizzly/http/HttpTrailer.java +++ b/modules/http/src/main/java/org/glassfish/grizzly/http/HttpTrailer.java @@ -34,7 +34,7 @@ public class HttpTrailer extends HttpContent implements MimeHeadersPacket { * @return true if passed {@link HttpContent} is a HttpTrailder. */ public static boolean isTrailer(HttpContent httpContent) { - return HttpTrailer.class.isAssignableFrom(httpContent.getClass()); + return httpContent != null && HttpTrailer.class.isAssignableFrom(httpContent.getClass()); } public static HttpTrailer create() { diff --git a/modules/http2/src/main/java/org/glassfish/grizzly/http2/DefaultOutputSink.java b/modules/http2/src/main/java/org/glassfish/grizzly/http2/DefaultOutputSink.java index b49cf30a7e..ac95e488e3 100644 --- a/modules/http2/src/main/java/org/glassfish/grizzly/http2/DefaultOutputSink.java +++ b/modules/http2/src/main/java/org/glassfish/grizzly/http2/DefaultOutputSink.java @@ -26,7 +26,6 @@ import java.util.List; import java.util.Map; import java.util.concurrent.atomic.AtomicInteger; -import java.util.concurrent.locks.ReentrantLock; import java.util.logging.Logger; import org.glassfish.grizzly.Buffer; @@ -166,7 +165,7 @@ public void onPeerWindowUpdate(final int delta) throws Http2StreamException { final Source resource = outputQueueRecord.resource; final HttpTrailer currentTrailer = outputQueueRecord.trailer; - final MessageCloner messageCloner = outputQueueRecord.cloner; + final MessageCloner messageCloner = outputQueueRecord.cloner; if (currentTrailer != null) { try { @@ -202,7 +201,7 @@ public void onPeerWindowUpdate(final int delta) throws Http2StreamException { final int dataChunkToSendSize = dataChunkToSend.remaining(); // send a http2 data frame - flushToConnectionOutputSink(null, dataChunkToSend, completionHandler, null, isLast); + flushToConnectionOutputSink(dataChunkToSend, completionHandler, isLast); // update the available window size bytes counter availStreamWindowSize.addAndGet(-dataChunkToSendSize); @@ -234,10 +233,10 @@ public void onPeerWindowUpdate(final int delta) throws Http2StreamException { * completely written in the current thread. * @throws IOException if an error occurs with the write operation. */ - @SuppressWarnings("ConstantConditions") @Override - public synchronized void writeDownStream(final HttpPacket httpPacket, final FilterChainContext ctx, final CompletionHandler completionHandler, - final MessageCloner messageCloner) throws IOException { + public synchronized void writeDownStream(final HttpPacket httpPacket, final FilterChainContext ctx, + final CompletionHandler completionHandler, final MessageCloner messageCloner) + throws IOException { assert ctx != null; assertReady(); @@ -248,18 +247,15 @@ public synchronized void writeDownStream(final HttpPacket httpPacket, final Filt List headerFrames = null; OutputQueueRecord outputQueueRecord = null; - final ReentrantLock deflatorLock = http2Session.getDeflaterLock(); - try { // try-finally block to release deflater lock if needed // If HTTP header hasn't been committed - commit it if (!httpHeader.isCommitted()) { // do we expect any HTTP payload? final boolean isNoPayload = !httpHeader.isExpectContent() - || httpContent != null && httpContent.isLast() && !httpContent.getContent().hasRemaining(); + || (httpContent != null && httpContent.isLast() && !httpContent.getContent().hasRemaining()); - // !!!!! LOCK the deflater - deflatorLock.lock(); + http2Session.getDeflaterLock().lock(); final boolean logging = NetLogger.isActive(); final Map capture = logging ? new HashMap<>() : null; headerFrames = http2Session.encodeHttpHeaderAsHeaderFrames(ctx, httpHeader, stream.getId(), isNoPayload, null, capture); @@ -280,7 +276,7 @@ public synchronized void writeDownStream(final HttpPacket httpPacket, final Filt response.acknowledged(); response.getHeaders().clear(); unflushedWritesCounter.incrementAndGet(); - flushToConnectionOutputSink(headerFrames, null, new FlushCompletionHandler(completionHandler), messageCloner, false); + flushToConnectionOutputSink(headerFrames, completionHandler, messageCloner, false); return; } } @@ -291,7 +287,7 @@ public synchronized void writeDownStream(final HttpPacket httpPacket, final Filt // if we don't expect any HTTP payload, mark this frame as // last and return unflushedWritesCounter.incrementAndGet(); - flushToConnectionOutputSink(headerFrames, null, new FlushCompletionHandler(completionHandler), messageCloner, isNoPayload); + flushToConnectionOutputSink(headerFrames, completionHandler, messageCloner, isNoPayload); return; } } @@ -303,13 +299,11 @@ public synchronized void writeDownStream(final HttpPacket httpPacket, final Filt http2Session.handlerFilter.onHttpContentEncoded(httpContent, ctx); - Buffer dataToSend = null; boolean isLast = httpContent.isLast(); final boolean isTrailer = HttpTrailer.isTrailer(httpContent); Buffer data = httpContent.getContent(); final int dataSize = data.remaining(); - unflushedWritesCounter.incrementAndGet(); final FlushCompletionHandler flushCompletionHandler = new FlushCompletionHandler(completionHandler); @@ -344,7 +338,10 @@ public synchronized void writeDownStream(final HttpPacket httpPacket, final Filt // check if our element wasn't forgotten (async) if (outputQueue.size() != spaceToReserve || !outputQueue.remove(outputQueueRecord)) { - // if not - return + // if not - send trailers and return + if (isLast && isTrailer) { + sendTrailers(completionHandler, messageCloner, (HttpTrailer) httpContent); + } return; } @@ -368,28 +365,17 @@ public synchronized void writeDownStream(final HttpPacket httpPacket, final Filt final Buffer dataChunkToStore = splitOutputBufferIfNeeded(data, fitWindowLen); // Create output record for the chunk to be stored - outputQueueRecord = new OutputQueueRecord(Source.factory(stream).createBufferSource(dataChunkToStore), flushCompletionHandler, isLast, - isZeroSizeData); + outputQueueRecord = new OutputQueueRecord( + Source.factory(stream).createBufferSource(dataChunkToStore), + flushCompletionHandler, isLast, isZeroSizeData); // reset completion handler and isLast for the current chunk isLast = false; } - // if there is a chunk to send - if (data != null && (data.hasRemaining() || isLast)) { - - final int dataChunkToSendSize = data.remaining(); - - // update the available window size bytes counter - availStreamWindowSize.addAndGet(-dataChunkToSendSize); - releaseWriteQueueSpace(dataChunkToSendSize, isZeroSizeData, outputQueueRecord == null); - - dataToSend = data; - } - // if there's anything to send - send it + final Buffer dataToSend = prepareDataToSend(outputQueueRecord == null, isLast, data, isZeroSizeData); if (headerFrames != null || dataToSend != null) { - // if another part of data is stored in the queue - // we have to increase CompletionHandler counter to avoid // premature notification @@ -397,30 +383,48 @@ public synchronized void writeDownStream(final HttpPacket httpPacket, final Filt outputQueueRecord.incChunksCounter(); } - flushToConnectionOutputSink(headerFrames, dataToSend, flushCompletionHandler, isDataCloned ? null : messageCloner, isLast && !isTrailer); + flushToConnectionOutputSink(headerFrames, dataToSend, flushCompletionHandler, + isDataCloned ? null : messageCloner, isLast && !isTrailer); } if (isLast) { if (isTrailer) { - // !!!!! LOCK the deflater sendTrailers(completionHandler, messageCloner, (HttpTrailer) httpContent); } return; } } finally { - if (deflatorLock.isHeldByCurrentThread()) { - deflatorLock.unlock(); + // if it is locked by this thread, it was locked in the first lines of this try block + if (http2Session.getDeflaterLock().isHeldByCurrentThread()) { + http2Session.getDeflaterLock().unlock(); } } if (outputQueueRecord == null) { return; } - addOutputQueueRecord(outputQueueRecord); } + + private Buffer prepareDataToSend(final boolean isRecordNull, final boolean isLast, final Buffer data, + final boolean isZeroSizeData) { + if (data == null) { + return null; + } + if (data.hasRemaining() || isLast) { + final int dataChunkToSendSize = data.remaining(); + // update the available window size bytes counter + availStreamWindowSize.addAndGet(-dataChunkToSendSize); + releaseWriteQueueSpace(dataChunkToSendSize, isZeroSizeData, isRecordNull); + return data; + } + return null; + } + + + /** * Flush {@link Http2Stream} output and notify {@link CompletionHandler} once all output data has been flushed. * @@ -455,8 +459,9 @@ public void flush(final CompletionHandler completionHandler) { } /** - * The method is responsible for checking the current output window size. The returned integer value is the size of the - * data, which could be sent now. + * The method is responsible for checking the current output window size. + * The returned integer value is the size of the data, which could be + * sent now. * * @param size check the provided size against the window size limit. * @@ -477,10 +482,30 @@ private Buffer splitOutputBufferIfNeeded(final Buffer buffer, final int length) return buffer.split(buffer.position() + length); } - private void flushToConnectionOutputSink(final List headerFrames, final Buffer data, final CompletionHandler completionHandler, - final MessageCloner messageCloner, final boolean isLast) { + private void flushToConnectionOutputSink(final List headerFrames, + final CompletionHandler completionHandler, + final MessageCloner messageCloner, + final boolean isLast) { + final FlushCompletionHandler flushCompletionHandler = new FlushCompletionHandler(completionHandler); + flushToConnectionOutputSink(headerFrames, null, flushCompletionHandler, messageCloner, isLast); + } - http2Session.getOutputSink().writeDataDownStream(stream, headerFrames, data, completionHandler, messageCloner, isLast); + private void flushToConnectionOutputSink( + final Buffer data, + final FlushCompletionHandler flushCompletionHandler, + final boolean isLast) { + flushToConnectionOutputSink(null, data, flushCompletionHandler, null, isLast); + } + + private void flushToConnectionOutputSink( + final List headerFrames, + final Buffer data, + final CompletionHandler completionHandler, + final MessageCloner messageCloner, + final boolean isLast) { + + http2Session.getOutputSink().writeDataDownStream( + stream, headerFrames, data, completionHandler, messageCloner, isLast); if (isLast) { terminate(OUT_FIN_TERMINATION); @@ -544,7 +569,7 @@ private boolean isTerminated() { private void writeEmptyFin() { if (!isTerminated()) { unflushedWritesCounter.incrementAndGet(); - flushToConnectionOutputSink(null, Buffers.EMPTY_BUFFER, new FlushCompletionHandler(null), null, true); + flushToConnectionOutputSink(Buffers.EMPTY_BUFFER, new FlushCompletionHandler(null), true); } } @@ -559,75 +584,66 @@ private boolean isWantToWrite() { } private void addOutputQueueRecord(OutputQueueRecord outputQueueRecord) throws Http2StreamException { - - do { // Make sure current outputQueueRecord is not forgotten - - // set the outputQueueRecord as the current + do { outputQueue.setCurrentElement(outputQueueRecord); // check if situation hasn't changed and we can't send the data chunk now - if (isWantToWrite() && outputQueue.compareAndSetCurrentElement(outputQueueRecord, null)) { - - // if we can send the output record now - do that - - final FlushCompletionHandler chunkedCompletionHandler = outputQueueRecord.chunkedCompletionHandler; + if (!isWantToWrite() || !outputQueue.compareAndSetCurrentElement(outputQueueRecord, null)) { + break; // will be (or already) written asynchronously + } - final HttpTrailer currentTrailer = outputQueueRecord.trailer; - final MessageCloner messageCloner = outputQueueRecord.cloner; - if (currentTrailer != null) { - try { - sendTrailers(chunkedCompletionHandler, messageCloner, currentTrailer); - } catch (IOException ex) { - LOGGER.log(WARNING, "Error sending trailers.", ex); - } - return; + // if we can send the output record now - do that + final FlushCompletionHandler chunkedCompletionHandler = outputQueueRecord.chunkedCompletionHandler; + final HttpTrailer currentTrailer = outputQueueRecord.trailer; + final MessageCloner messageCloner = outputQueueRecord.cloner; + if (currentTrailer != null) { + try { + sendTrailers(chunkedCompletionHandler, messageCloner, currentTrailer); + } catch (IOException ex) { + LOGGER.log(WARNING, "Error sending trailers.", ex); } + return; + } - boolean isLast = outputQueueRecord.isLast; - final boolean isZeroSizeData = outputQueueRecord.isZeroSizeData; + boolean isLast = outputQueueRecord.isLast; + final boolean isZeroSizeData = outputQueueRecord.isZeroSizeData; + final Source currentResource = outputQueueRecord.resource; - final Source currentResource = outputQueueRecord.resource; + final int fitWindowLen = checkOutputWindow(currentResource.remaining()); + final Buffer dataChunkToSend = currentResource.read(fitWindowLen); - final int fitWindowLen = checkOutputWindow(currentResource.remaining()); - final Buffer dataChunkToSend = currentResource.read(fitWindowLen); + // if there is a chunk to store + if (currentResource.hasRemaining()) { + // Create output record for the chunk to be stored + outputQueueRecord.reset(currentResource, chunkedCompletionHandler, isLast); + outputQueueRecord.incChunksCounter(); - // if there is a chunk to store - if (currentResource.hasRemaining()) { - // Create output record for the chunk to be stored - outputQueueRecord.reset(currentResource, chunkedCompletionHandler, isLast); - outputQueueRecord.incChunksCounter(); + // reset isLast for the current chunk + isLast = false; + } else { + outputQueueRecord.release(); + outputQueueRecord = null; + } - // reset isLast for the current chunk - isLast = false; - } else { - outputQueueRecord.release(); - outputQueueRecord = null; - } + // if there is a chunk to send + if (dataChunkToSend != null && (dataChunkToSend.hasRemaining() || isLast)) { + final int dataChunkToSendSize = dataChunkToSend.remaining(); + flushToConnectionOutputSink(dataChunkToSend, chunkedCompletionHandler, isLast); - // if there is a chunk to send - if (dataChunkToSend != null && (dataChunkToSend.hasRemaining() || isLast)) { - final int dataChunkToSendSize = dataChunkToSend.remaining(); - - flushToConnectionOutputSink(null, dataChunkToSend, chunkedCompletionHandler, null, isLast); - - // update the available window size bytes counter - availStreamWindowSize.addAndGet(-dataChunkToSendSize); - releaseWriteQueueSpace(dataChunkToSendSize, isZeroSizeData, outputQueueRecord == null); - - } else if (isZeroSizeData && outputQueueRecord == null) { - // if it's atomic and no remainder left - don't forget to release ATOMIC_QUEUE_RECORD_SIZE - releaseWriteQueueSpace(0, true, true); - } else if (dataChunkToSend != null && !dataChunkToSend.hasRemaining()) { - // current window won't allow the data to be sent. Will be written once the - // window changes. - if (outputQueueRecord != null) { - reserveWriteQueueSpace(outputQueueRecord.resource.remaining()); - outputQueue.offer(outputQueueRecord); - } - break; + // update the available window size bytes counter + availStreamWindowSize.addAndGet(-dataChunkToSendSize); + releaseWriteQueueSpace(dataChunkToSendSize, isZeroSizeData, outputQueueRecord == null); + } else if (isZeroSizeData && outputQueueRecord == null) { + // if it's atomic and no remainder left - don't forget to release ATOMIC_QUEUE_RECORD_SIZE + releaseWriteQueueSpace(0, true, true); + } else if (dataChunkToSend != null && !dataChunkToSend.hasRemaining()) { + // current window won't allow the data to be sent. Will be written once the + // window changes. + if (outputQueueRecord != null) { + reserveWriteQueueSpace(outputQueueRecord.resource.remaining()); + outputQueue.offer(outputQueueRecord); } - } else { - break; // will be (or already) written asynchronously + break; } } while (outputQueueRecord != null); } @@ -644,22 +660,31 @@ private void releaseWriteQueueSpace(final int justSentBytes, final boolean isAto } } - private void sendTrailers(final CompletionHandler completionHandler, final MessageCloner messageCloner, final HttpTrailer httpContent) - throws IOException { - final boolean logging = NetLogger.isActive(); - final Map capture = logging ? new HashMap<>() : null; - List trailerFrames = http2Session.encodeTrailersAsHeaderFrames(stream.getId(), new ArrayList<>(4), httpContent.getHeaders(), capture); - if (logging) { - for (Http2Frame http2Frame : trailerFrames) { - if (http2Frame.getType() == PushPromiseFrame.TYPE) { - NetLogger.log(NetLogger.Context.TX, http2Session, (HeadersFrame) http2Frame, capture); - break; + private void sendTrailers(final CompletionHandler completionHandler, + final MessageCloner messageCloner, final HttpTrailer httpContent) + throws IOException { + http2Session.getDeflaterLock().lock(); + try { + final boolean logging = NetLogger.isActive(); + final Map capture = logging ? new HashMap<>() : null; + final List trailerFrames = + http2Session.encodeTrailersAsHeaderFrames(stream.getId(), + new ArrayList<>(4), + httpContent.getHeaders(), capture); + if (logging) { + for (Http2Frame http2Frame : trailerFrames) { + if (http2Frame.getType() == PushPromiseFrame.TYPE) { + NetLogger.log(NetLogger.Context.TX, http2Session, (HeadersFrame) http2Frame, capture); + break; + } } } + unflushedWritesCounter.incrementAndGet(); + flushToConnectionOutputSink(trailerFrames, completionHandler, messageCloner, true); + close(); + } finally { + http2Session.getDeflaterLock().unlock(); } - unflushedWritesCounter.incrementAndGet(); - flushToConnectionOutputSink(trailerFrames, null, new FlushCompletionHandler(completionHandler), messageCloner, true); - close(); } private static class OutputQueueRecord extends AsyncQueueRecord { @@ -667,13 +692,14 @@ private static class OutputQueueRecord extends AsyncQueueRecord { private FlushCompletionHandler chunkedCompletionHandler; private HttpTrailer trailer; - private MessageCloner cloner; + private MessageCloner cloner; private boolean isLast; private final boolean isZeroSizeData; - public OutputQueueRecord(final Source resource, final FlushCompletionHandler completionHandler, final boolean isLast, final boolean isZeroSizeData) { + public OutputQueueRecord(final Source resource, final FlushCompletionHandler completionHandler, + final boolean isLast, final boolean isZeroSizeData) { super(null, null, null); this.resource = resource; @@ -737,11 +763,13 @@ public WriteResult getCurrentResult() { } /** - * Flush {@link CompletionHandler}, which will be passed on each {@link Http2Stream} write to make sure the data reached - * the wires. + * Flush {@link CompletionHandler}, which will be passed on each {@link Http2Stream} write to make sure + * the data reached the wires. + * + * Usually FlushCompletionHandler is also used as a wrapper for custom {@link CompletionHandler} + * provided by users. * - * Usually FlushCompletionHandler is also used as a wrapper for custom {@link CompletionHandler} provided by - * users. + * The parent class has an internal state, so be careful with reuses of the same instance! */ private final class FlushCompletionHandler extends ChunkedCompletionHandler { @@ -749,6 +777,9 @@ public FlushCompletionHandler(final CompletionHandler parentComplet super(parentCompletionHandler); } + /** + * Notifies all flush handlers about the completition. + */ @Override protected void done0() { synchronized (flushHandlersSync) { // synchronize with flush() diff --git a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2AddOn.java b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2AddOn.java index a9bb1d2bc2..e7187a398a 100644 --- a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2AddOn.java +++ b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2AddOn.java @@ -44,7 +44,7 @@ public class Http2AddOn implements AddOn { // ----------------------------------------------------------- Constructors - @SuppressWarnings("unused") + public Http2AddOn() { this(Http2Configuration.builder().build()); } @@ -57,6 +57,7 @@ public Http2AddOn(final Http2Configuration http2Configuration) { @Override public void setup(NetworkListener networkListener, FilterChainBuilder builder) { + LOGGER.config(() -> String.format("setup(networkListener=%s, builder=%s)", networkListener, builder)); final TCPNIOTransport transport = networkListener.getTransport(); if (networkListener.isSecure() && !AlpnSupport.isEnabled()) { @@ -83,25 +84,26 @@ public Http2Configuration getConfiguration() { // -------------------------------------------------------- Private Methods private Http2ServerFilter updateFilterChain(final FilterChainBuilder builder) { - final int codecFilterIdx = builder.indexOfType(org.glassfish.grizzly.http.HttpServerFilter.class); - final Http2ServerFilter http2HandlerFilter = new Http2ServerFilter(http2Configuration); - http2HandlerFilter.setLocalMaxFramePayloadSize(http2Configuration.getMaxFramePayloadSize()); builder.add(codecFilterIdx + 1, http2HandlerFilter); - return http2HandlerFilter; } - private static void configureAlpn(final Transport transport, final Http2ServerFilter http2Filter, final FilterChainBuilder builder) { + private static void configureAlpn(final Transport transport, + final Http2ServerFilter http2Filter, + final FilterChainBuilder builder) { + LOGGER.finest(() -> String.format("configureAlpn(transport=%s, http2Filter=%s, builder=%s)", + transport, http2Filter, builder)); final int idx = builder.indexOfType(SSLBaseFilter.class); - if (idx != -1) { - final SSLBaseFilter sslFilter = (SSLBaseFilter) builder.get(idx); - - AlpnSupport.getInstance().configure(sslFilter); - AlpnSupport.getInstance().setServerSideNegotiator(transport, new AlpnServerNegotiatorImpl(http2Filter)); + if (idx == -1) { + LOGGER.warning("No usable SSLBaseFilter found!"); + return; } + final SSLBaseFilter sslFilter = (SSLBaseFilter) builder.get(idx); + AlpnSupport.getInstance().configure(sslFilter); + AlpnSupport.getInstance().setServerSideNegotiator(transport, new AlpnServerNegotiatorImpl(http2Filter)); } } diff --git a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2ClientFilter.java b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2ClientFilter.java index 513dd3533f..af6b2ff14f 100644 --- a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2ClientFilter.java +++ b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2ClientFilter.java @@ -27,6 +27,7 @@ import java.util.List; import java.util.Map; import java.util.concurrent.locks.ReentrantLock; +import java.util.logging.Logger; import javax.net.ssl.SSLEngine; @@ -70,6 +71,8 @@ * @author oleksiys */ public class Http2ClientFilter extends Http2BaseFilter { + + private static final Logger LOGGER = Logger.getLogger(Http2ClientFilter.class.getName()); private final AlpnClientNegotiatorImpl defaultClientAlpnNegotiator; private boolean isNeverForceUpgrade; @@ -80,7 +83,6 @@ public class Http2ClientFilter extends Http2BaseFilter { public Http2ClientFilter(final Http2Configuration configuration) { super(configuration); defaultClientAlpnNegotiator = new AlpnClientNegotiatorImpl(this); - defaultHttp2Upgrade = HeaderValue.newHeaderValue(HTTP2_CLEAR); connectionUpgradeHeaderValue = HeaderValue.newHeaderValue("Upgrade, HTTP2-Settings"); } @@ -88,7 +90,6 @@ public Http2ClientFilter(final Http2Configuration configuration) { /** * @return true if an upgrade to HTTP/2 will not be performed, otherwise false */ - @SuppressWarnings("unused") public boolean isNeverForceUpgrade() { return isNeverForceUpgrade; } @@ -98,7 +99,6 @@ public boolean isNeverForceUpgrade() { * * @param neverForceUpgrade true to disable upgrade attempts, otherwise false */ - @SuppressWarnings("unused") public void setNeverForceUpgrade(boolean neverForceUpgrade) { this.isNeverForceUpgrade = neverForceUpgrade; } @@ -107,7 +107,6 @@ public void setNeverForceUpgrade(boolean neverForceUpgrade) { * @return true if the push request has to be sent upstream, so a user have a chance to process it, or * false otherwise */ - @SuppressWarnings("unused") public boolean isSendPushRequestUpstream() { return sendPushRequestUpstream; } @@ -116,20 +115,20 @@ public boolean isSendPushRequestUpstream() { * @param sendPushRequestUpstream true if the push request has to be sent upstream, so a user have a chance to * process it, or false otherwise */ - @SuppressWarnings("unused") public void setSendPushRequestUpstream(boolean sendPushRequestUpstream) { this.sendPushRequestUpstream = sendPushRequestUpstream; } @Override public NextAction handleConnect(final FilterChainContext ctx) throws IOException { + LOGGER.finest(() -> String.format("handleConnect(ctx=%s)", ctx)); final Connection connection = ctx.getConnection(); - final FilterChain filterChain = (FilterChain) connection.getProcessor(); final int idx = filterChain.indexOfType(SSLFilter.class); if (idx != -1) { // use TLS ALPN final SSLFilter sslFilter = (SSLFilter) filterChain.get(idx); + LOGGER.finest(() -> String.format("Using AlpnSupport for filter: %s", sslFilter)); AlpnSupport.getInstance().configure(sslFilter); AlpnSupport.getInstance().setClientSideNegotiator(connection, getClientAlpnNegotiator()); @@ -153,6 +152,7 @@ public void failed(Throwable throwable) { connection.enableIOEvent(IOEvent.READ); return suspendAction; } else if (getConfiguration().isPriorKnowledge()) { + LOGGER.finest(() -> String.format("Using HTTP 1.1 upgrade mechanism for connection: %s", connection)); final Http2Session http2Session = createClientHttp2Session(connection); final Http2State state = http2Session.getHttp2State(); state.setDirectUpgradePhase(); @@ -174,13 +174,13 @@ public void ready(Http2Session http2Session) { return ctx.getInvokeAction(); } - @SuppressWarnings("unchecked") @Override public NextAction handleRead(final FilterChainContext ctx) throws IOException { - + LOGGER.finest(() -> String.format("handleRead(ctx=%s)", ctx)); // if it's a stream chain (the stream is already assigned) - just // bypass the parsing part if (checkIfHttp2StreamChain(ctx)) { + LOGGER.finest("Already registered HTTP2 stream chain, invoking action."); return ctx.getInvokeAction(); } @@ -188,6 +188,7 @@ public NextAction handleRead(final FilterChainContext ctx) throws IOException { Http2State http2State = Http2State.get(connection); if (http2State == null || http2State.isNeverHttp2()) { + LOGGER.finest("Not a HTTP2 connection, invoking action."); // NOT HTTP2 connection and never will be return ctx.getInvokeAction(); } @@ -202,7 +203,7 @@ public NextAction handleRead(final FilterChainContext ctx) throws IOException { final HttpRequestPacket httpRequest = httpResponse.getRequest(); if (!tryHttpUpgrade(ctx, http2State, httpRequest, httpResponse)) { - // upgrade didn't work out + LOGGER.finest("Upgrade to HTTP2 didn't work out. Invoking action."); http2State.setNeverHttp2(); return ctx.getInvokeAction(); } @@ -225,6 +226,7 @@ public NextAction handleRead(final FilterChainContext ctx) throws IOException { @Override public NextAction handleWrite(final FilterChainContext ctx) throws IOException { + LOGGER.finest(() -> String.format("handleWrite(ctx=%s)", ctx)); final Connection connection = ctx.getConnection(); Http2State http2State = Http2State.get(connection); @@ -244,30 +246,29 @@ public NextAction handleWrite(final FilterChainContext ctx) throws IOException { assert HttpPacket.isHttp(ctx.getMessage()); checkIfLastHttp11Chunk(ctx, http2State, msg); - return ctx.getInvokeAction(); - } else { - if (http2State.isHttpUpgradePhase()) { - // We still don't have the server response regarding HTTP2 upgrade offer - final Object msg = ctx.getMessage(); - if (HttpPacket.isHttp(msg)) { - if (!((HttpPacket) msg).getHttpHeader().isCommitted()) { - throw new IllegalStateException("Can't pipeline HTTP requests because it's still not clear if HTTP/1.x or HTTP/2 will be used"); - } + } - checkIfLastHttp11Chunk(ctx, http2State, msg); + if (http2State.isHttpUpgradePhase()) { + // We still don't have the server response regarding HTTP2 upgrade offer + final Object msg = ctx.getMessage(); + if (HttpPacket.isHttp(msg)) { + if (!((HttpPacket) msg).getHttpHeader().isCommitted()) { + throw new IllegalStateException("Can't pipeline HTTP requests because it's still not clear if HTTP/1.x or HTTP/2 will be used"); } - return ctx.getInvokeAction(); + checkIfLastHttp11Chunk(ctx, http2State, msg); } + + return ctx.getInvokeAction(); } return super.handleWrite(ctx); } @Override - @SuppressWarnings("unchecked") public NextAction handleEvent(final FilterChainContext ctx, final FilterChainEvent event) throws IOException { + LOGGER.finest(() -> String.format("handleEvent(ctx=%s, event=%s)", ctx, event)); if (!Http2State.isHttp2(ctx.getConnection())) { return ctx.getInvokeAction(); } @@ -368,7 +369,8 @@ protected AlpnClientNegotiator getClientAlpnNegotiator() { } /** - * The method is called once a client receives an HTTP response to its initial propose to establish HTTP/2.0 connection. + * The method is called once a client receives an HTTP response to its initial propose to establish + * HTTP/2.0 connection. * * @param ctx the current {@link FilterChainContext} * @param http2State the HTTP2 connection state @@ -424,7 +426,6 @@ private boolean tryHttpUpgrade(final FilterChainContext ctx, final Http2State ht } final Http2Stream stream = http2Session.openUpgradeStream(httpRequest, 0); - final HttpContext oldHttpContext = httpResponse.getProcessingState().getHttpContext(); // replace the HttpContext @@ -432,9 +433,11 @@ private boolean tryHttpUpgrade(final FilterChainContext ctx, final Http2State ht httpRequest.getProcessingState().setHttpContext(httpContext); httpContext.attach(ctx); - final HttpRequestPacket dummyRequestPacket = HttpRequestPacket.builder().method(Method.PRI).uri("/dummy_pri").protocol(Protocol.HTTP_2_0).build(); + final HttpRequestPacket dummyRequestPacket = HttpRequestPacket.builder() + .method(Method.PRI).uri("/dummy_pri").protocol(Protocol.HTTP_2_0).build(); - final HttpResponsePacket dummyResponsePacket = HttpResponsePacket.builder(dummyRequestPacket).status(200).reasonPhrase("OK").protocol(Protocol.HTTP_2_0) + final HttpResponsePacket dummyResponsePacket = HttpResponsePacket.builder(dummyRequestPacket) + .status(200).reasonPhrase("OK").protocol(Protocol.HTTP_2_0) .build(); dummyResponsePacket.getProcessingState().setHttpContext(oldHttpContext); diff --git a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2ServerFilter.java b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2ServerFilter.java index 5ad54b6314..c4adc03e20 100644 --- a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2ServerFilter.java +++ b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2ServerFilter.java @@ -176,8 +176,8 @@ public class Http2ServerFilter extends Http2BaseFilter { private final Attribute CIPHER_CHECKED = AttributeBuilder.DEFAULT_ATTRIBUTE_BUILDER.createAttribute("BLACK_LIST_CIPHER_SUITE_CHEKCED"); - private Collection activeConnections = new HashSet<>(1024); - private AtomicBoolean shuttingDown = new AtomicBoolean(); + private final Collection activeConnections = new HashSet<>(1024); + private final AtomicBoolean shuttingDown = new AtomicBoolean(); /** * Create a new {@link Http2ServerFilter} using the specified {@link Http2Configuration}. Configuration may be changed @@ -188,24 +188,26 @@ public Http2ServerFilter(final Http2Configuration configuration) { } /** - * The flag, which enables/disables payload support for HTTP methods, for which HTTP spec doesn't clearly state whether - * they support payload. Known "undefined" methods are: GET, HEAD, DELETE. + * The flag, which enables/disables payload support for HTTP methods, for which HTTP spec + * doesn't clearly state whether they support payload. + *

+ * Known "undefined" methods are: GET, HEAD, DELETE. * * @return true if "undefined" methods support payload, or false otherwise */ - @SuppressWarnings("unused") public boolean isAllowPayloadForUndefinedHttpMethods() { return allowPayloadForUndefinedHttpMethods; } /** - * The flag, which enables/disables payload support for HTTP methods, for which HTTP spec doesn't clearly state whether - * they support payload. Known "undefined" methods are: GET, HEAD, DELETE. + * The flag, which enables/disables payload support for HTTP methods, for which HTTP spec + * doesn't clearly state whether they support payload. + *

+ * Known "undefined" methods are: GET, HEAD, DELETE. * - * @param allowPayloadForUndefinedHttpMethods true if "undefined" methods support payload, or false - * otherwise + * @param allowPayloadForUndefinedHttpMethods true if "undefined" methods support + * payload, or false otherwise */ - @SuppressWarnings("unused") public void setAllowPayloadForUndefinedHttpMethods(boolean allowPayloadForUndefinedHttpMethods) { this.allowPayloadForUndefinedHttpMethods = allowPayloadForUndefinedHttpMethods; } @@ -226,13 +228,13 @@ public NextAction handleClose(final FilterChainContext ctx) throws IOException { return ctx.getInvokeAction(); } - @SuppressWarnings("unchecked") @Override public NextAction handleRead(final FilterChainContext ctx) throws IOException { - + LOGGER.finest(() -> String.format("handleRead(ctx=%s)", ctx)); // if it's a stream chain (the stream is already assigned) - just // bypass the parsing part if (checkIfHttp2StreamChain(ctx)) { + LOGGER.finest("Already registered HTTP2 stream chain, invoking action."); return ctx.getInvokeAction(); } @@ -240,7 +242,7 @@ public NextAction handleRead(final FilterChainContext ctx) throws IOException { Http2State http2State = Http2State.get(connection); if (http2State != null && http2State.isNeverHttp2()) { - // NOT HTTP2 connection and never will be + LOGGER.finest("Not a HTTP2 connection, invoking action."); return ctx.getInvokeAction(); } @@ -254,24 +256,24 @@ public NextAction handleRead(final FilterChainContext ctx) throws IOException { // ALPN should've set the Http2State, but in our case it's null. // It means ALPN was bypassed - SSL without ALPN shouldn't work. // Don't try HTTP/2 in this case. + LOGGER.finest("Secure connection, but http2State was null, ALPN was bypassed. Invoking action."); Http2State.create(connection).setNeverHttp2(); return ctx.getInvokeAction(); } final HttpRequestPacket httpRequest = (HttpRequestPacket) httpHeader; - if (!Method.PRI.equals(httpRequest.getMethod())) { + if (Method.PRI.equals(httpRequest.getMethod())) { + // PRI method + // DIRECT HTTP/2.0 request + http2State = doDirectUpgrade(ctx); + } else { final boolean isLast = httpContent.isLast(); if (tryHttpUpgrade(ctx, httpRequest, isLast) && isLast) { enableOpReadNow(ctx); } - return ctx.getInvokeAction(); } - - // PRI method - // DIRECT HTTP/2.0 request - http2State = doDirectUpgrade(ctx); } final Http2Session http2Session = obtainHttp2Session(http2State, ctx, true); @@ -346,6 +348,7 @@ public NextAction handleRead(final FilterChainContext ctx) throws IOException { @Override public NextAction handleEvent(final FilterChainContext ctx, final FilterChainEvent event) throws IOException { + LOGGER.finest(() -> String.format("handleEvent(ctx=%s, event=%s)", ctx, event)); final Object type = event.type(); if (type == ShutdownEvent.TYPE) { @@ -431,6 +434,7 @@ protected void onPrefaceReceived(Http2Session http2Session) { } private Http2State doDirectUpgrade(final FilterChainContext ctx) { + LOGGER.finest(() -> String.format("doDirectUpgrade(ctx=%s)", ctx)); final Connection connection = ctx.getConnection(); final Http2Session http2Session = new Http2Session(connection, true, this); @@ -452,7 +456,8 @@ Collection shuttingDown() { return activeConnections; } - private boolean tryHttpUpgrade(final FilterChainContext ctx, final HttpRequestPacket httpRequest, final boolean isLast) throws Http2StreamException { + private boolean tryHttpUpgrade(final FilterChainContext ctx, final HttpRequestPacket httpRequest, final boolean isLast) + throws Http2StreamException { if (!checkHttpMethodOnUpgrade(httpRequest)) { return false; @@ -740,6 +745,7 @@ protected void processOutgoingHttpHeader(final FilterChainContext ctx, final Htt } private void doPush(final FilterChainContext ctx, final PushEvent pushEvent) { + LOGGER.finest(() -> String.format("doPush(ctx=%s, pushEvent=%s)", ctx, pushEvent)); final Http2Session http2Session = Http2Session.get(ctx.getConnection()); if (http2Session == null) { throw new IllegalStateException("Unable to find valid Http2Session"); diff --git a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2Session.java b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2Session.java index 01ef91b00f..40a34b8f61 100644 --- a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2Session.java +++ b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2Session.java @@ -390,7 +390,6 @@ public final int getLocalMaxFramePayloadSize() { /** * @return The max payload size to be accepted by the peer */ - @SuppressWarnings("unused") public int getPeerMaxFramePayloadSize() { return peerMaxFramePayloadSize; } @@ -443,12 +442,10 @@ public int getLocalConnectionWindowSize() { return localConnectionWindowSize; } - @SuppressWarnings("unused") public void setLocalConnectionWindowSize(final int localConnectionWindowSize) { this.localConnectionWindowSize = localConnectionWindowSize; } - @SuppressWarnings("unused") public int getAvailablePeerConnectionWindowSize() { return outputSink.getAvailablePeerConnectionWindowSize(); } @@ -872,7 +869,7 @@ private List bufferToPushPromiseFrames(final int streamId, final int private List completeHeadersProviderFrameSerialization(final HeaderBlockFragment.HeaderBlockFragmentBuilder builder, final int streamId, final Buffer compressedHeaders, List toList) { // we assume deflaterLock is acquired and held by this thread - assert deflaterLock.isHeldByCurrentThread(); + assert getDeflaterLock().isHeldByCurrentThread(); if (toList == null) { toList = tmpHeaderFramesList; diff --git a/modules/http2/src/main/java/org/glassfish/grizzly/http2/utils/ChunkedCompletionHandler.java b/modules/http2/src/main/java/org/glassfish/grizzly/http2/utils/ChunkedCompletionHandler.java index 65faf0e477..4c447a8d2f 100644 --- a/modules/http2/src/main/java/org/glassfish/grizzly/http2/utils/ChunkedCompletionHandler.java +++ b/modules/http2/src/main/java/org/glassfish/grizzly/http2/utils/ChunkedCompletionHandler.java @@ -16,6 +16,8 @@ package org.glassfish.grizzly.http2.utils; +import java.util.logging.Logger; + import org.glassfish.grizzly.CompletionHandler; import org.glassfish.grizzly.WriteResult; @@ -24,12 +26,16 @@ * @author oleksiys */ public class ChunkedCompletionHandler implements CompletionHandler { + private static final Logger LOG = Logger.getLogger(ChunkedCompletionHandler.class.getName()); private final CompletionHandler parentCompletionHandler; private boolean isDone; - protected int chunksCounter = 1; + private int chunksCounter = 1; private long writtenSize; + /** + * @param parentCompletionHandler - can be null + */ public ChunkedCompletionHandler(final CompletionHandler parentCompletionHandler) { this.parentCompletionHandler = parentCompletionHandler; } @@ -40,6 +46,7 @@ public void incChunks() { @Override public void cancelled() { + LOG.finest("cancelled()"); if (done()) { if (parentCompletionHandler != null) { parentCompletionHandler.cancelled(); @@ -49,6 +56,8 @@ public void cancelled() { @Override public void failed(Throwable throwable) { + // we don't need a stacktrace here, but we want to see why we are here. + LOG.finest(() -> String.format("failed(throwable=%s)", throwable)); if (done()) { if (parentCompletionHandler != null) { parentCompletionHandler.failed(throwable); @@ -58,6 +67,7 @@ public void failed(Throwable throwable) { @Override public void completed(final WriteResult result) { + LOG.finest(() -> String.format("completed(result=%s)", result)); if (isDone) { return; } @@ -84,9 +94,9 @@ public void completed(final WriteResult result) { @Override public void updated(final WriteResult result) { + LOG.finest(() -> String.format("updated(result=%s)", result)); if (parentCompletionHandler != null) { final long initialWrittenSize = result.getWrittenSize(); - try { result.setWrittenSize(writtenSize + initialWrittenSize); parentCompletionHandler.updated(result); @@ -107,6 +117,11 @@ private boolean done() { return true; } + + /** + * This method does nothing but can be overriden to implement some action executed before + * the parent completition handler is executed. + */ protected void done0() { } } diff --git a/modules/http2/src/test/java/org/glassfish/grizzly/http2/TrailersTest.java b/modules/http2/src/test/java/org/glassfish/grizzly/http2/TrailersTest.java index a4ac9807d8..addf538db0 100644 --- a/modules/http2/src/test/java/org/glassfish/grizzly/http2/TrailersTest.java +++ b/modules/http2/src/test/java/org/glassfish/grizzly/http2/TrailersTest.java @@ -137,7 +137,7 @@ public NextAction handleRead(FilterChainContext ctx) throws IOException { return ctx.getStopAction(); } }; - final Connection c = getConnection("localhost", PORT, filter); + final Connection c = getConnection("localhost", PORT, filter); HttpRequestPacket.Builder builder = HttpRequestPacket.builder(); HttpRequestPacket request = builder.method(Method.POST).uri("/echo").protocol(Protocol.HTTP_2_0).host("localhost:" + PORT).build(); c.write(HttpTrailer.builder(request).content(Buffers.wrap(MemoryManager.DEFAULT_MEMORY_MANAGER, "a=b&c=d")).last(true).header("trailer-a", "value-a") @@ -196,7 +196,7 @@ public NextAction handleRead(FilterChainContext ctx) throws IOException { return ctx.getStopAction(); } }; - final Connection c = getConnection("localhost", PORT, filter); + final Connection c = getConnection("localhost", PORT, filter); HttpRequestPacket.Builder builder = HttpRequestPacket.builder(); HttpRequestPacket request = builder.method(Method.POST).uri("/echo").protocol(Protocol.HTTP_2_0).host("localhost:" + PORT).build(); c.write(HttpContent.builder(request) // write the request diff --git a/modules/http2/src/test/resources/logging.properties b/modules/http2/src/test/resources/logging.properties index c280aa86dd..35d02b11e1 100644 --- a/modules/http2/src/test/resources/logging.properties +++ b/modules/http2/src/test/resources/logging.properties @@ -2,12 +2,14 @@ handlers=java.util.logging.ConsoleHandler java.util.logging.ConsoleHandler.encoding=UTF-8 java.util.logging.ConsoleHandler.level=ALL +java.util.logging.ConsoleHandler.formatter=org.glassfish.grizzly.utils.LoggingFormatter # Note: NIOOutputSinksTest fails with too verbose logging .level=INFO -org.glassfish.grizzly.http2.level=INFO org.glassfish.grizzly.filterchain.DefaultFilterChain.level=INFO +org.glassfish.grizzly.http.server.Request.level=INFO +org.glassfish.grizzly.http2.level=INFO org.glassfish.grizzly.http2.DefaultInputBuffer.level=INFO org.glassfish.grizzly.http2.Http2Stream.level=INFO -org.glassfish.grizzly.http2.NetLogger.level=FINE +org.glassfish.grizzly.http2.NetLogger.level=INFO From fccd33dc821cc705170ea19f923ac27ca110ac86 Mon Sep 17 00:00:00 2001 From: David Matejcek Date: Sun, 6 Sep 2020 22:25:06 +0200 Subject: [PATCH 11/26] Issue #2016 Better compatibility detection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - can run with any compatible implementation, not only NPN Bootstrap - with new JDK versions after 8u250 uses it's JSSE impl by default - still can use older NPN bootstrap versions if configured - can use also other implementations (openjsse) Signed-off-by: David Matějček --- .../glassfish/grizzly/http2/AlpnSupport.java | 68 +++------ .../http2/AplnExtensionCompatibility.java | 144 ++++++++++++++++++ .../glassfish/grizzly/http2/Http2AddOn.java | 3 +- .../src/test/resources/logging.properties | 1 + 4 files changed, 171 insertions(+), 45 deletions(-) create mode 100644 modules/http2/src/main/java/org/glassfish/grizzly/http2/AplnExtensionCompatibility.java diff --git a/modules/http2/src/main/java/org/glassfish/grizzly/http2/AlpnSupport.java b/modules/http2/src/main/java/org/glassfish/grizzly/http2/AlpnSupport.java index 275d6efd09..2cd4b622da 100644 --- a/modules/http2/src/main/java/org/glassfish/grizzly/http2/AlpnSupport.java +++ b/modules/http2/src/main/java/org/glassfish/grizzly/http2/AlpnSupport.java @@ -26,7 +26,6 @@ import java.util.WeakHashMap; import java.util.concurrent.locks.ReadWriteLock; import java.util.concurrent.locks.ReentrantReadWriteLock; -import java.util.function.BiFunction; import java.util.logging.Level; import java.util.logging.Logger; @@ -47,36 +46,18 @@ /** * Grizzly TLS Next Protocol Negotiation support class. - * */ public class AlpnSupport { - private final static Logger LOGGER = Grizzly.logger(AlpnSupport.class); - - private final static Map> SSL_TO_CONNECTION_MAP = new WeakHashMap<>(); + private static final Logger LOGGER = Grizzly.logger(AlpnSupport.class); + private static final Map> SSL_TO_CONNECTION_MAP = new WeakHashMap<>(); private static final AlpnSupport INSTANCE; - private static final Method nativeHandshakeMethod; + private static final AplnExtensionCompatibility COMPATIBILITY; static { - boolean isExtensionFound = false; - Method setHandshakeAlpnSelector = null; - - try { - setHandshakeAlpnSelector = SSLEngine.class.getMethod("setHandshakeApplicationProtocolSelector", BiFunction.class); - } catch (Exception e) { - try { - ClassLoader.getSystemClassLoader().loadClass("sun.security.ssl.GrizzlyNPN"); - isExtensionFound = true; - } catch (Exception e2) { - LOGGER.log(Level.FINE, "Native ALPN is not found:", e); - LOGGER.log(Level.FINE, "TLS ALPN extension is not found:", e2); - } - } - - nativeHandshakeMethod = setHandshakeAlpnSelector; - INSTANCE = isExtensionFound - || nativeHandshakeMethod != null - ? new AlpnSupport() : null; + COMPATIBILITY = AplnExtensionCompatibility.getInstance(); + LOGGER.config(() -> "Detected ALPN compatibility info: " + COMPATIBILITY); + INSTANCE = COMPATIBILITY.isAlpnExtensionAvailable() ? new AlpnSupport() : null; } public static boolean isEnabled() { @@ -87,7 +68,6 @@ public static AlpnSupport getInstance() { if (!isEnabled()) { throw new IllegalStateException("TLS ALPN is disabled"); } - return INSTANCE; } @@ -114,17 +94,24 @@ private static void setConnection(final SSLEngine engine, final Connection co @Override public void onInit(final Connection connection, final SSLEngine sslEngine) { assert sslEngine != null; - - AlpnServerNegotiator negotiator = getServerNegotiator(connection); - - if (negotiator != null && nativeHandshakeMethod != null) { - // Code only works for JDK9+ - // sslEngine.setHandshakeApplicationProtocolSelector(negotiator); - try { - nativeHandshakeMethod.invoke(sslEngine, negotiator); - } catch (Exception ex) { - LOGGER.log(Level.SEVERE, "Couldn't execute sslEngine.setHandshakeApplicationProtocolSelector", ex); - } + if (sslEngine.getUseClientMode()) { + // makes sense only for the server + return; + } + if (!COMPATIBILITY.isProtocolSelectorSetterInImpl()) { + // even when the api implements it, impl doesn't + return; + } + final AlpnServerNegotiator negotiator = getServerNegotiator(connection); + if (negotiator == null) { + return; + } + // Older JDK8 versions are missing this method in API, that's why we do this. + final Method setter = COMPATIBILITY.getProtocolSelectorSetter(sslEngine); + try { + setter.invoke(sslEngine, negotiator); + } catch (Exception ex) { + LOGGER.log(Level.SEVERE, "Couldn't execute " + setter, ex); } } @@ -135,7 +122,6 @@ public void onStart(final Connection connection) { if (sslEngine.getUseClientMode()) { AlpnClientNegotiator negotiator = getClientNegotiator(connection); - if (negotiator != null) { // add a CloseListener to ensure we remove the // negotiator associated with this SSLEngine @@ -151,9 +137,7 @@ public void onClosed(Closeable closeable, CloseType type) throws IOException { } } else { AlpnServerNegotiator negotiator = getServerNegotiator(connection); - if (negotiator != null) { - // add a CloseListener to ensure we remove the // negotiator associated with this SSLEngine connection.addCloseListener(new CloseListener() { @@ -179,7 +163,6 @@ public void onFailure(Connection connection, Throwable t) { } }; - private AlpnSupport() { } @@ -205,7 +188,6 @@ public void setClientSideNegotiator(final Connection connection, final AlpnCl private void putServerSideNegotiator(final Object object, final AlpnServerNegotiator negotiator) { serverSideLock.writeLock().lock(); - try { serverSideNegotiators.put(object, negotiator); } finally { @@ -226,7 +208,6 @@ private void putClientSideNegotiator(final Object object, final AlpnClientNegoti private AlpnClientNegotiator getClientNegotiator(Connection connection) { AlpnClientNegotiator negotiator; clientSideLock.readLock().lock(); - try { negotiator = clientSideNegotiators.get(connection); if (negotiator == null) { @@ -242,7 +223,6 @@ private AlpnClientNegotiator getClientNegotiator(Connection connection) { private AlpnServerNegotiator getServerNegotiator(Connection connection) { AlpnServerNegotiator negotiator; serverSideLock.readLock().lock(); - try { negotiator = serverSideNegotiators.get(connection); if (negotiator == null) { diff --git a/modules/http2/src/main/java/org/glassfish/grizzly/http2/AplnExtensionCompatibility.java b/modules/http2/src/main/java/org/glassfish/grizzly/http2/AplnExtensionCompatibility.java new file mode 100644 index 0000000000..8becb07696 --- /dev/null +++ b/modules/http2/src/main/java/org/glassfish/grizzly/http2/AplnExtensionCompatibility.java @@ -0,0 +1,144 @@ +/* + * Copyright (c) 2012, 2017 Oracle and/or its affiliates. All rights reserved. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License v. 2.0, which is available at + * http://www.eclipse.org/legal/epl-2.0. + * + * This Source Code may also be made available under the following Secondary + * Licenses when the conditions for such availability set forth in the + * Eclipse Public License v. 2.0 are satisfied: GNU General Public License, + * version 2 with the GNU Classpath Exception, which is available at + * https://www.gnu.org/software/classpath/license.html. + * + * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 + */ + +package org.glassfish.grizzly.http2; + +import java.lang.reflect.Method; +import java.util.Objects; +import java.util.function.BiFunction; +import java.util.logging.Logger; + +import javax.net.ssl.SSLEngine; + +class AplnExtensionCompatibility { + private static final Logger LOG = Logger.getLogger(AplnExtensionCompatibility.class.getName()); + + private static final String IMPL_CLASS_NAME = "sun.security.ssl.SSLEngineImpl"; + private static final String METHOD_NAME = "setHandshakeApplicationProtocolSelector"; + + private static AplnExtensionCompatibility INSTANCE; + + private final boolean alpnExtensionGrizzly; + private final boolean protocolSelectorSetterInApi; + private final boolean protocolSelectorSetterInImpl; + + public static synchronized AplnExtensionCompatibility getInstance() { + if (INSTANCE == null) { + INSTANCE = new AplnExtensionCompatibility(); + } + return INSTANCE; + } + + + public boolean isAlpnExtensionAvailable() { + return isAlpnExtensionGrizzly() || isProtocolSelectorSetterInApi() || isProtocolSelectorSetterInImpl(); + } + + + public boolean isAlpnExtensionGrizzly() { + return alpnExtensionGrizzly; + } + + + public boolean isProtocolSelectorSetterInApi() { + return protocolSelectorSetterInApi; + } + + + public boolean isProtocolSelectorSetterInImpl() { + return protocolSelectorSetterInImpl; + } + + + public Method getProtocolSelectorSetter(final SSLEngine engine) { + Objects.requireNonNull(engine, "engine"); + try { + // newer JSSE versions implement this method. + // some JDK8 versions (Zulu 8u265) don't see the method as public on impl + final Class engineClass; + if (isHandshakeSetterInApi()) { + engineClass = SSLEngine.class; + } else { + engineClass = engine.getClass(); + } + return engineClass.getMethod(METHOD_NAME, BiFunction.class); + } catch (final NoSuchMethodException e) { + throw new IllegalArgumentException("The method public void setHandshakeApplicationProtocolSelector(" + + "BiFunction, String> selector) is not declared by" + + " the " + engine.getClass().getName() + ".", e); + } + } + + + private AplnExtensionCompatibility() { + this.alpnExtensionGrizzly = isClassAvailableOnBootstrapClasspath("sun.security.ssl.GrizzlyNPN"); + this.protocolSelectorSetterInApi = isHandshakeSetterInApi(); + this.protocolSelectorSetterInImpl = isHandshakeSetterInImpl(); + } + + + private static boolean isClassAvailableOnBootstrapClasspath(final String className) { + try { + ClassLoader.getSystemClassLoader().loadClass(className); + return true; + } catch (final ClassNotFoundException e) { + LOG.config("The class with the name '" + className + "' is not available on the bootstrap classpath."); + return false; + } + } + + + private static boolean isHandshakeSetterInImpl() { + try { + Class.forName(IMPL_CLASS_NAME).getMethod(METHOD_NAME, BiFunction.class); + return true; + } catch (final IllegalAccessError e) { + LOG.config(() -> "The class " + IMPL_CLASS_NAME + " is not accessible."); + return false; + } catch (final ClassNotFoundException | NoClassDefFoundError e) { + LOG.config(() -> "The class " + IMPL_CLASS_NAME + " cloud not be found."); + return false; + } catch (final NoSuchMethodException e) { + LOG.config(() -> "The method public void setHandshakeApplicationProtocolSelector(" + + "BiFunction, String> selector) is not declared by" + + " the " + IMPL_CLASS_NAME + " class."); + return false; + } + } + + + private static boolean isHandshakeSetterInApi() { + try { + // new grizzly bootstrap versions implement this method. + SSLEngine.class.getMethod(METHOD_NAME, BiFunction.class); + return true; + } catch (final NoSuchMethodException e) { + LOG.config("The method public void setHandshakeApplicationProtocolSelector(" + + "BiFunction, String> selector) is not declared by" + + " the " + SSLEngine.class.getName() + "."); + return false; + } + } + + + @Override + public String toString() { + return super.toString() + "ALPN available: " + isAlpnExtensionAvailable() + + ", ALPN is Grizzly: " + isAlpnExtensionGrizzly() + + ", setHandshakeApplicationProtocolSelector in API: " + isProtocolSelectorSetterInApi() + + ", setHandshakeApplicationProtocolSelector in impl: " + isProtocolSelectorSetterInImpl(); + } +} diff --git a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2AddOn.java b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2AddOn.java index e7187a398a..e94c164b5c 100644 --- a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2AddOn.java +++ b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2AddOn.java @@ -61,7 +61,8 @@ public void setup(NetworkListener networkListener, FilterChainBuilder builder) { final TCPNIOTransport transport = networkListener.getTransport(); if (networkListener.isSecure() && !AlpnSupport.isEnabled()) { - LOGGER.warning("TLS ALPN (Application-Layer Protocol Negotiation) support is not available. HTTP/2 support will not be enabled."); + LOGGER.warning("TLS ALPN (Application-Layer Protocol Negotiation) support is not available." + + " HTTP/2 support will not be enabled."); return; } diff --git a/modules/http2/src/test/resources/logging.properties b/modules/http2/src/test/resources/logging.properties index 35d02b11e1..bc4a950136 100644 --- a/modules/http2/src/test/resources/logging.properties +++ b/modules/http2/src/test/resources/logging.properties @@ -10,6 +10,7 @@ java.util.logging.ConsoleHandler.formatter=org.glassfish.grizzly.utils.LoggingFo org.glassfish.grizzly.filterchain.DefaultFilterChain.level=INFO org.glassfish.grizzly.http.server.Request.level=INFO org.glassfish.grizzly.http2.level=INFO +org.glassfish.grizzly.http2.AlpnSupport.level=CONFIG org.glassfish.grizzly.http2.DefaultInputBuffer.level=INFO org.glassfish.grizzly.http2.Http2Stream.level=INFO org.glassfish.grizzly.http2.NetLogger.level=INFO From fef6f988896adf2c7c8671f05eef356642f6b334 Mon Sep 17 00:00:00 2001 From: David Matejcek Date: Tue, 22 Sep 2020 12:08:57 +0200 Subject: [PATCH 12/26] Issue #2016 Better logging, reliable locking, variable names, but ... MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - in this state grizzly: - fails TrailersTest (grizzly-http2; race condition, passes if I use stepping) - passes TCK tests with Payara (websocket) - still fails h2spec with Payara (race condition, different, but related issue) Signed-off-by: David Matějček --- .../grizzly/http2/DefaultOutputSink.java | 190 +++++++++--------- .../grizzly/http2/Http2SessionOutputSink.java | 21 +- .../src/test/resources/logging.properties | 1 + 3 files changed, 106 insertions(+), 106 deletions(-) diff --git a/modules/http2/src/main/java/org/glassfish/grizzly/http2/DefaultOutputSink.java b/modules/http2/src/main/java/org/glassfish/grizzly/http2/DefaultOutputSink.java index ac95e488e3..6daeae9a6b 100644 --- a/modules/http2/src/main/java/org/glassfish/grizzly/http2/DefaultOutputSink.java +++ b/modules/http2/src/main/java/org/glassfish/grizzly/http2/DefaultOutputSink.java @@ -57,7 +57,7 @@ * @author Alexey Stashok */ class DefaultOutputSink implements StreamOutputSink { - private static final Logger LOGGER = Grizzly.logger(StreamOutputSink.class); + private static final Logger LOGGER = Grizzly.logger(DefaultOutputSink.class); private static final int MAX_OUTPUT_QUEUE_SIZE = 65536; @@ -165,15 +165,9 @@ public void onPeerWindowUpdate(final int delta) throws Http2StreamException { final Source resource = outputQueueRecord.resource; final HttpTrailer currentTrailer = outputQueueRecord.trailer; - final MessageCloner messageCloner = outputQueueRecord.cloner; - if (currentTrailer != null) { - try { - outputQueueRecord = null; - sendTrailers(completionHandler, messageCloner, currentTrailer); - } catch (IOException ex) { - LOGGER.log(WARNING, "Error sending trailers.", ex); - } + outputQueueRecord = null; + sendTrailers(completionHandler, currentTrailer); return; } @@ -221,7 +215,6 @@ public void onPeerWindowUpdate(final int delta) throws Http2StreamException { } } } - /** * Send an {@link HttpPacket} to the {@link Http2Stream}. * @@ -238,27 +231,42 @@ public synchronized void writeDownStream(final HttpPacket httpPacket, final Filt final CompletionHandler completionHandler, final MessageCloner messageCloner) throws IOException { assert ctx != null; - assertReady(); + final OutputQueueRecord next = writeDownStream0(httpPacket, ctx, completionHandler, messageCloner); + if (next != null) { + addOutputQueueRecord(next); + } + } + + + private OutputQueueRecord writeDownStream0(final HttpPacket httpPacket, + final FilterChainContext ctx, + final CompletionHandler completionHandler, + final MessageCloner messageCloner) + throws IOException { final HttpHeader httpHeader = stream.getOutputHttpHeader(); final HttpContent httpContent = HttpContent.isContent(httpPacket) ? (HttpContent) httpPacket : null; - List headerFrames = null; - OutputQueueRecord outputQueueRecord = null; - - try { // try-finally block to release deflater lock if needed + boolean sendTrailers = false; + boolean lockedByMe = false; + try { + // try-finally block to release deflater lock if needed + boolean isLast = httpContent != null && httpContent.isLast(); + final boolean isTrailer = HttpTrailer.isTrailer(httpContent); // If HTTP header hasn't been committed - commit it if (!httpHeader.isCommitted()) { - // do we expect any HTTP payload? - final boolean isNoPayload = !httpHeader.isExpectContent() + final boolean dontSendPayload = !httpHeader.isExpectContent() || (httpContent != null && httpContent.isLast() && !httpContent.getContent().hasRemaining()); + LOGGER.finest(() -> "Header not committed yet; dontSendPayload=" + dontSendPayload); http2Session.getDeflaterLock().lock(); + lockedByMe = true; final boolean logging = NetLogger.isActive(); final Map capture = logging ? new HashMap<>() : null; - headerFrames = http2Session.encodeHttpHeaderAsHeaderFrames(ctx, httpHeader, stream.getId(), isNoPayload, null, capture); + headerFrames = http2Session.encodeHttpHeaderAsHeaderFrames( + ctx, httpHeader, stream.getId(), dontSendPayload, null, capture); if (logging) { for (Http2Frame http2Frame : headerFrames) { if (http2Frame.getType() == PushPromiseFrame.TYPE) { @@ -267,7 +275,7 @@ public synchronized void writeDownStream(final HttpPacket httpPacket, final Filt } } } - stream.onSndHeaders(isNoPayload); + stream.onSndHeaders(dontSendPayload); // 100-Continue block if (!httpHeader.isRequest()) { @@ -277,75 +285,61 @@ public synchronized void writeDownStream(final HttpPacket httpPacket, final Filt response.getHeaders().clear(); unflushedWritesCounter.incrementAndGet(); flushToConnectionOutputSink(headerFrames, completionHandler, messageCloner, false); - return; + LOGGER.finest("Acknowledgement has been sent."); + return null; } } httpHeader.setCommitted(true); - if (isNoPayload || httpContent == null) { - // if we don't expect any HTTP payload, mark this frame as - // last and return + if (dontSendPayload || httpContent == null) { + // if we don't expect any HTTP payload, mark this frame as last and return unflushedWritesCounter.incrementAndGet(); - flushToConnectionOutputSink(headerFrames, completionHandler, messageCloner, isNoPayload); - return; + flushToConnectionOutputSink(headerFrames, completionHandler, messageCloner, dontSendPayload); + sendTrailers = false; + LOGGER.finest(() -> "Nothing to send; dontSendPayload=" + dontSendPayload); + return null; } } - // if there is nothing to write - return if (httpContent == null) { - return; + sendTrailers = false; + LOGGER.finest("Nothing to send, httpContent is null."); + return null; } http2Session.handlerFilter.onHttpContentEncoded(httpContent, ctx); - boolean isLast = httpContent.isLast(); - final boolean isTrailer = HttpTrailer.isTrailer(httpContent); Buffer data = httpContent.getContent(); final int dataSize = data.remaining(); unflushedWritesCounter.incrementAndGet(); final FlushCompletionHandler flushCompletionHandler = new FlushCompletionHandler(completionHandler); - - boolean isDataCloned = false; - final boolean isZeroSizeData = dataSize == 0; final int spaceToReserve = isZeroSizeData ? ZERO_QUEUE_RECORD_SIZE : dataSize; + boolean isDataCloned = false; // Check if output queue is not empty - add new element - if (reserveWriteQueueSpace(spaceToReserve) > spaceToReserve) { + final int spaceReserved = reserveWriteQueueSpace(spaceToReserve); + LOGGER.finest(() -> "Bytes reserved: " + spaceReserved + ", was requested: " + spaceToReserve); + if (spaceReserved > spaceToReserve) { // if the queue is not empty - the headers should have been sent assert headerFrames == null; - if (messageCloner != null) { data = messageCloner.clone(http2Session.getConnection(), data); isDataCloned = true; } - if (isTrailer) { - outputQueueRecord = new OutputQueueRecord( - Source.factory(stream).createBufferSource(data), - flushCompletionHandler, - (HttpTrailer) httpContent, - false); - } else { - outputQueueRecord = new OutputQueueRecord( - Source.factory(stream).createBufferSource(data), - flushCompletionHandler, isLast, isZeroSizeData); - } - - outputQueue.offer(outputQueueRecord); + final OutputQueueRecord record = createOutputQueueRecord(data, httpContent, flushCompletionHandler, + isLast, isZeroSizeData); + outputQueue.offer(record); // check if our element wasn't forgotten (async) - if (outputQueue.size() != spaceToReserve || !outputQueue.remove(outputQueueRecord)) { - // if not - send trailers and return - if (isLast && isTrailer) { - sendTrailers(completionHandler, messageCloner, (HttpTrailer) httpContent); - } - return; + if (outputQueue.size() != spaceToReserve || !outputQueue.remove(record)) { + sendTrailers = false; + LOGGER.finest("In some weird condition. FIXME... why are we removing what we added in previous if/else?"); + return null; } - - outputQueueRecord = null; } // our element is first in the output queue @@ -354,9 +348,13 @@ public synchronized void writeDownStream(final HttpPacket httpPacket, final Filt // check if output record's buffer is fitting into window size // if not - split it into 2 parts: part to send, part to keep in the queue final int fitWindowLen = checkOutputWindow(remaining); + LOGGER.finest(() -> "Remaining: " + remaining + ", fitWindowLen: " + fitWindowLen); - // if there is a chunk to store - if (fitWindowLen < remaining) { + final OutputQueueRecord outputQueueRecord; + if (fitWindowLen >= remaining) { + outputQueueRecord = null; + } else { + // if there is a chunk to store if (!isDataCloned && messageCloner != null) { data = messageCloner.clone(http2Session.getConnection(), data); isDataCloned = true; @@ -365,9 +363,8 @@ public synchronized void writeDownStream(final HttpPacket httpPacket, final Filt final Buffer dataChunkToStore = splitOutputBufferIfNeeded(data, fitWindowLen); // Create output record for the chunk to be stored - outputQueueRecord = new OutputQueueRecord( - Source.factory(stream).createBufferSource(dataChunkToStore), - flushCompletionHandler, isLast, isZeroSizeData); + outputQueueRecord = createOutputQueueRecord(dataChunkToStore, null, flushCompletionHandler, isLast, + isZeroSizeData); // reset completion handler and isLast for the current chunk isLast = false; @@ -377,34 +374,36 @@ public synchronized void writeDownStream(final HttpPacket httpPacket, final Filt final Buffer dataToSend = prepareDataToSend(outputQueueRecord == null, isLast, data, isZeroSizeData); if (headerFrames != null || dataToSend != null) { // if another part of data is stored in the queue - - // we have to increase CompletionHandler counter to avoid - // premature notification + // we have to increase CompletionHandler counter to avoid premature notification if (outputQueueRecord != null) { outputQueueRecord.incChunksCounter(); } - flushToConnectionOutputSink(headerFrames, dataToSend, flushCompletionHandler, isDataCloned ? null : messageCloner, isLast && !isTrailer); } - if (isLast) { - if (isTrailer) { - sendTrailers(completionHandler, messageCloner, (HttpTrailer) httpContent); - } - return; - } - + LOGGER.finest("isLast=" + isLast + "; isTrailer=" + isTrailer); + sendTrailers = isLast && isTrailer; + return isLast ? null : outputQueueRecord; } finally { - // if it is locked by this thread, it was locked in the first lines of this try block - if (http2Session.getDeflaterLock().isHeldByCurrentThread()) { + LOGGER.finest("sendTrailers=" + sendTrailers); + if (sendTrailers) { + sendTrailers(completionHandler, (HttpTrailer) httpContent); + } + if (lockedByMe) { http2Session.getDeflaterLock().unlock(); } } + } + - if (outputQueueRecord == null) { - return; + private OutputQueueRecord createOutputQueueRecord(final Buffer data, final HttpContent httpContent, + final FlushCompletionHandler flushCompletionHandler, boolean isLast, final boolean isZeroSizeData) { + final Source bufferSource = Source.factory(stream).createBufferSource(data); + if (httpContent instanceof HttpTrailer) { + return new OutputQueueRecord(bufferSource, flushCompletionHandler, (HttpTrailer) httpContent, false); } - addOutputQueueRecord(outputQueueRecord); + return new OutputQueueRecord(bufferSource, flushCompletionHandler, isLast, isZeroSizeData); } @@ -485,16 +484,16 @@ private Buffer splitOutputBufferIfNeeded(final Buffer buffer, final int length) private void flushToConnectionOutputSink(final List headerFrames, final CompletionHandler completionHandler, final MessageCloner messageCloner, - final boolean isLast) { + final boolean sendFIN) { final FlushCompletionHandler flushCompletionHandler = new FlushCompletionHandler(completionHandler); - flushToConnectionOutputSink(headerFrames, null, flushCompletionHandler, messageCloner, isLast); + flushToConnectionOutputSink(headerFrames, null, flushCompletionHandler, messageCloner, sendFIN); } private void flushToConnectionOutputSink( final Buffer data, final FlushCompletionHandler flushCompletionHandler, - final boolean isLast) { - flushToConnectionOutputSink(null, data, flushCompletionHandler, null, isLast); + final boolean sendFIN) { + flushToConnectionOutputSink(null, data, flushCompletionHandler, null, sendFIN); } private void flushToConnectionOutputSink( @@ -502,12 +501,12 @@ private void flushToConnectionOutputSink( final Buffer data, final CompletionHandler completionHandler, final MessageCloner messageCloner, - final boolean isLast) { + final boolean sendFIN) { http2Session.getOutputSink().writeDataDownStream( - stream, headerFrames, data, completionHandler, messageCloner, isLast); + stream, headerFrames, data, completionHandler, messageCloner, sendFIN); - if (isLast) { + if (sendFIN) { terminate(OUT_FIN_TERMINATION); } } @@ -595,13 +594,8 @@ private void addOutputQueueRecord(OutputQueueRecord outputQueueRecord) throws Ht // if we can send the output record now - do that final FlushCompletionHandler chunkedCompletionHandler = outputQueueRecord.chunkedCompletionHandler; final HttpTrailer currentTrailer = outputQueueRecord.trailer; - final MessageCloner messageCloner = outputQueueRecord.cloner; if (currentTrailer != null) { - try { - sendTrailers(chunkedCompletionHandler, messageCloner, currentTrailer); - } catch (IOException ex) { - LOGGER.log(WARNING, "Error sending trailers.", ex); - } + sendTrailers(chunkedCompletionHandler, currentTrailer); return; } @@ -660,9 +654,7 @@ private void releaseWriteQueueSpace(final int justSentBytes, final boolean isAto } } - private void sendTrailers(final CompletionHandler completionHandler, - final MessageCloner messageCloner, final HttpTrailer httpContent) - throws IOException { + private void sendTrailers(final CompletionHandler completionHandler, final HttpTrailer httpContent) { http2Session.getDeflaterLock().lock(); try { final boolean logging = NetLogger.isActive(); @@ -679,24 +671,25 @@ private void sendTrailers(final CompletionHandler completionHandler } } } + flushToConnectionOutputSink(trailerFrames, completionHandler, null, true); unflushedWritesCounter.incrementAndGet(); - flushToConnectionOutputSink(trailerFrames, completionHandler, messageCloner, true); - close(); + } catch (IOException ex) { + LOGGER.log(WARNING, "Error sending trailers.", ex); } finally { + close(); + LOGGER.finest("Sending trailers finished, unlocking the deflater lock ..."); http2Session.getDeflaterLock().unlock(); } } private static class OutputQueueRecord extends AsyncQueueRecord { + private final HttpTrailer trailer; + private final boolean isZeroSizeData; + private Source resource; private FlushCompletionHandler chunkedCompletionHandler; - - private HttpTrailer trailer; - private MessageCloner cloner; - private boolean isLast; - private final boolean isZeroSizeData; public OutputQueueRecord(final Source resource, final FlushCompletionHandler completionHandler, final boolean isLast, final boolean isZeroSizeData) { @@ -705,6 +698,7 @@ public OutputQueueRecord(final Source resource, final FlushCompletionHandler com this.resource = resource; this.chunkedCompletionHandler = completionHandler; this.isLast = isLast; + this.trailer = null; this.isZeroSizeData = isZeroSizeData; } diff --git a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2SessionOutputSink.java b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2SessionOutputSink.java index 2935ae54d3..086f2ad89d 100644 --- a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2SessionOutputSink.java +++ b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2SessionOutputSink.java @@ -40,8 +40,8 @@ import org.glassfish.grizzly.http2.frames.Http2Frame; /** - * Class represents an output sink associated with specific {@link Http2Session} and is responsible for session - * (connection) level flow control. + * Class represents an output sink associated with specific {@link Http2Session} + * and is responsible for session (connection) level flow control. * * @author Alexey Stashok */ @@ -77,18 +77,24 @@ protected Http2FrameCodec frameCodec() { protected void writeDownStream(final Http2Frame frame) { - http2Session.getHttp2SessionChain().write(http2Session.getConnection(), null, frameCodec().serializeAndRecycle(http2Session, frame), null, - (MessageCloner) null); + http2Session.getHttp2SessionChain().write( + http2Session.getConnection(), null, + frameCodec().serializeAndRecycle(http2Session, frame), + null, (MessageCloner) null); } protected void writeDownStream(final List frames) { - http2Session.getHttp2SessionChain().write(http2Session.getConnection(), null, frameCodec().serializeAndRecycle(http2Session, frames), null, - (MessageCloner) null); + http2Session.getHttp2SessionChain().write( + http2Session.getConnection(), null, + frameCodec().serializeAndRecycle(http2Session, frames), + null, (MessageCloner) null); } @SuppressWarnings("unchecked") - protected void writeDownStream(final K anyMessage, final CompletionHandler completionHandler, final MessageCloner messageCloner) { + protected void writeDownStream(final K anyMessage, + final CompletionHandler completionHandler, + final MessageCloner messageCloner) { // Encode Http2Frame -> Buffer final Object msg; @@ -151,7 +157,6 @@ protected void writeDataDownStream(final Http2Stream stream, final List Date: Thu, 19 Nov 2020 08:31:37 +0100 Subject: [PATCH 13/26] Issue #2016 Using different port in tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 19001 is often occupied in Kubuntu Signed-off-by: David Matějček --- .../java/org/glassfish/grizzly/http/HttpResponseParseTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/http/src/test/java/org/glassfish/grizzly/http/HttpResponseParseTest.java b/modules/http/src/test/java/org/glassfish/grizzly/http/HttpResponseParseTest.java index 29239adede..250d3836a7 100644 --- a/modules/http/src/test/java/org/glassfish/grizzly/http/HttpResponseParseTest.java +++ b/modules/http/src/test/java/org/glassfish/grizzly/http/HttpResponseParseTest.java @@ -58,7 +58,7 @@ public class HttpResponseParseTest extends TestCase { private static final Logger logger = Grizzly.logger(HttpResponseParseTest.class); - public static final int PORT = 19001; + public static final int PORT = 19021; public void testHeaderlessResponseLine() throws Exception { doHttpResponseTest("HTTP/1.0", 200, "OK", Collections.>emptyMap(), "\r\n"); From 1d3501765f707a6e10aa3d7c819e7b57fe6e7987 Mon Sep 17 00:00:00 2001 From: David Matejcek Date: Thu, 19 Nov 2020 08:40:55 +0100 Subject: [PATCH 14/26] Issue #2016 Fixed TrailersTest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - if the queue size is not same as reserved space, and if it is last content and the content is a trailer, send trailers - if the queue size is same as reserved space, and record was already processed, don't send trailers. - TrailersTest - just formatting and finals Signed-off-by: David Matějček --- .../grizzly/http2/DefaultOutputSink.java | 12 +++-- .../glassfish/grizzly/http2/TrailersTest.java | 50 ++++++++++--------- 2 files changed, 34 insertions(+), 28 deletions(-) diff --git a/modules/http2/src/main/java/org/glassfish/grizzly/http2/DefaultOutputSink.java b/modules/http2/src/main/java/org/glassfish/grizzly/http2/DefaultOutputSink.java index 6daeae9a6b..2cabe3f161 100644 --- a/modules/http2/src/main/java/org/glassfish/grizzly/http2/DefaultOutputSink.java +++ b/modules/http2/src/main/java/org/glassfish/grizzly/http2/DefaultOutputSink.java @@ -251,7 +251,6 @@ private OutputQueueRecord writeDownStream0(final HttpPacket httpPacket, boolean sendTrailers = false; boolean lockedByMe = false; try { - // try-finally block to release deflater lock if needed boolean isLast = httpContent != null && httpContent.isLast(); final boolean isTrailer = HttpTrailer.isTrailer(httpContent); @@ -334,10 +333,15 @@ private OutputQueueRecord writeDownStream0(final HttpPacket httpPacket, isLast, isZeroSizeData); outputQueue.offer(record); - // check if our element wasn't forgotten (async) - if (outputQueue.size() != spaceToReserve || !outputQueue.remove(record)) { + // there is yet something in the queue before current record + if (outputQueue.size() != spaceToReserve) { + sendTrailers = isLast && isTrailer; + return null; + } + // + if (!outputQueue.remove(record)) { sendTrailers = false; - LOGGER.finest("In some weird condition. FIXME... why are we removing what we added in previous if/else?"); + LOGGER.finest("The record has been already processed."); return null; } } diff --git a/modules/http2/src/test/java/org/glassfish/grizzly/http2/TrailersTest.java b/modules/http2/src/test/java/org/glassfish/grizzly/http2/TrailersTest.java index addf538db0..c4e1f29b5f 100644 --- a/modules/http2/src/test/java/org/glassfish/grizzly/http2/TrailersTest.java +++ b/modules/http2/src/test/java/org/glassfish/grizzly/http2/TrailersTest.java @@ -52,6 +52,7 @@ import org.glassfish.grizzly.memory.Buffers; import org.glassfish.grizzly.memory.MemoryManager; import org.glassfish.grizzly.nio.transport.TCPNIOConnectorHandler; +import org.glassfish.grizzly.nio.transport.TCPNIOTransport; import org.junit.After; import org.junit.Test; @@ -75,10 +76,10 @@ public void testTrailers() throws Exception { configureHttpServer(); startHttpServer(new HttpHandler() { @Override - public void service(Request request, Response response) throws Exception { + public void service(final Request request, final Response response) throws Exception { response.setContentType("text/plain"); final InputStream in = request.getInputStream(); - StringBuilder sb = new StringBuilder(); + final StringBuilder sb = new StringBuilder(); int b; while ((b = in.read()) != -1) { sb.append((char) b); @@ -103,7 +104,7 @@ public Map get() { final Filter filter = new BaseFilter() { @Override - public NextAction handleRead(FilterChainContext ctx) throws IOException { + public NextAction handleRead(final FilterChainContext ctx) throws IOException { final HttpContent httpContent = ctx.getMessage(); try { if (lastProcessed.get()) { @@ -120,7 +121,7 @@ public NextAction handleRead(FilterChainContext ctx) throws IOException { latch.countDown(); } else { assertFalse(httpContent instanceof HttpTrailer); - int result = contentCount.incrementAndGet(); + final int result = contentCount.incrementAndGet(); if (result == 1) { assertTrue(httpContent.getContent().remaining() == 0); // response } else if (result == 2) { @@ -129,7 +130,7 @@ public NextAction handleRead(FilterChainContext ctx) throws IOException { fail("Unexpected content"); } } - } catch (Throwable t) { + } catch (final Throwable t) { error.set(t); latch.countDown(); } @@ -138,10 +139,11 @@ public NextAction handleRead(FilterChainContext ctx) throws IOException { } }; final Connection c = getConnection("localhost", PORT, filter); - HttpRequestPacket.Builder builder = HttpRequestPacket.builder(); - HttpRequestPacket request = builder.method(Method.POST).uri("/echo").protocol(Protocol.HTTP_2_0).host("localhost:" + PORT).build(); - c.write(HttpTrailer.builder(request).content(Buffers.wrap(MemoryManager.DEFAULT_MEMORY_MANAGER, "a=b&c=d")).last(true).header("trailer-a", "value-a") - .header("trailer-b", "value-b").build()); + final HttpRequestPacket.Builder builder = HttpRequestPacket.builder(); + final HttpRequestPacket request = builder.method(Method.POST).uri("/echo") + .protocol(Protocol.HTTP_2_0).host("localhost:" + PORT).build(); + c.write(HttpTrailer.builder(request).content(Buffers.wrap(MemoryManager.DEFAULT_MEMORY_MANAGER, "a=b&c=d")) + .last(true).header("trailer-a", "value-a").header("trailer-b", "value-b").build()); assertTrue(latch.await(10, TimeUnit.SECONDS)); final Throwable t = error.get(); if (t != null) { @@ -156,7 +158,7 @@ public void testNoContentTrailers() throws Exception { configureHttpServer(); startHttpServer(new HttpHandler() { @Override - public void service(Request request, Response response) throws Exception { + public void service(final Request request, final Response response) throws Exception { response.setContentType("text/plain"); final InputStream in = request.getInputStream(); // noinspection StatementWithEmptyBody @@ -176,20 +178,19 @@ public Map get() { final Filter filter = new BaseFilter() { @Override - public NextAction handleRead(FilterChainContext ctx) throws IOException { + public NextAction handleRead(final FilterChainContext ctx) throws IOException { final HttpContent httpContent = ctx.getMessage(); try { if (httpContent.isLast()) { assertTrue(httpContent instanceof HttpTrailer); final MimeHeaders trailers = ((HttpTrailer) httpContent).getHeaders(); - assertEquals(2, trailers.size()); assertEquals("value-a", trailers.getHeader("trailer-a")); assertEquals("value-b", trailers.getHeader("trailer-b")); - latch.countDown(); } - } catch (Throwable t) { + } catch (final Throwable t) { error.set(t); + } finally { latch.countDown(); } @@ -197,12 +198,14 @@ public NextAction handleRead(FilterChainContext ctx) throws IOException { } }; final Connection c = getConnection("localhost", PORT, filter); - HttpRequestPacket.Builder builder = HttpRequestPacket.builder(); - HttpRequestPacket request = builder.method(Method.POST).uri("/echo").protocol(Protocol.HTTP_2_0).host("localhost:" + PORT).build(); - c.write(HttpContent.builder(request) // write the request - .last(false).build()); - c.write(HttpTrailer.builder(request) // write the trailer - .content(Buffers.EMPTY_BUFFER).last(true).header("trailer-a", "value-a").header("trailer-b", "value-b").build()); + final HttpRequestPacket.Builder builder = HttpRequestPacket.builder(); + final HttpRequestPacket request = builder.method(Method.POST).uri("/echo") + .protocol(Protocol.HTTP_2_0).host("localhost:" + PORT).build(); + // write the request + c.write(HttpContent.builder(request).last(false).build()); + // write the trailer + c.write(HttpTrailer.builder(request).content(Buffers.EMPTY_BUFFER).last(true) + .header("trailer-a", "value-a").header("trailer-b", "value-b").build()); assertTrue(latch.await(10, TimeUnit.SECONDS)); final Throwable t = error.get(); if (t != null) { @@ -227,10 +230,9 @@ private Connection getConnection(final String host, final int port, final Filter final FilterChain clientChain = createClientFilterChainAsBuilder(false, true, clientFilter).build(); - SocketConnectorHandler connectorHandler = TCPNIOConnectorHandler.builder(httpServer.getListener("grizzly").getTransport()).processor(clientChain) - .build(); - - Future connectFuture = connectorHandler.connect(host, port); + final TCPNIOTransport transport = httpServer.getListener("grizzly").getTransport(); + final SocketConnectorHandler connectorHandler = TCPNIOConnectorHandler.builder(transport).processor(clientChain).build(); + final Future connectFuture = connectorHandler.connect(host, port); return connectFuture.get(10, TimeUnit.SECONDS); } From 17f83521ccb42da820dfb7c8b6d480424f9c84da Mon Sep 17 00:00:00 2001 From: David Matejcek Date: Thu, 19 Nov 2020 23:05:57 +0100 Subject: [PATCH 15/26] Issue #2016 CloseReason has toString now MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: David Matějček --- .../main/java/org/glassfish/grizzly/CloseReason.java | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/modules/grizzly/src/main/java/org/glassfish/grizzly/CloseReason.java b/modules/grizzly/src/main/java/org/glassfish/grizzly/CloseReason.java index 839e70e238..0abd633729 100644 --- a/modules/grizzly/src/main/java/org/glassfish/grizzly/CloseReason.java +++ b/modules/grizzly/src/main/java/org/glassfish/grizzly/CloseReason.java @@ -64,10 +64,19 @@ public CloseType getType() { * * If the cause wasn't specified by user and it was closed locally then {@link #LOCALLY_CLOSED} will be returned. If the * cause wasn't specified by user and it was closed remotely then {@link #REMOTELY_CLOSED} will be returned. - * + * * @return information about an error, that caused the {@link Connection} to be closed */ public IOException getCause() { return cause; } + + + /** + * Returns also type and cause. + */ + @Override + public String toString() { + return super.toString() + "[type="+ getType() + ", cause=" + getCause() + "]"; + } } From 5767b374df6a7929c6db1a9d49e22e4b795be69d Mon Sep 17 00:00:00 2001 From: David Matejcek Date: Thu, 19 Nov 2020 23:35:50 +0100 Subject: [PATCH 16/26] Issue #2016 Minor syntax changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: David Matějček --- .../grizzly/asyncqueue/TaskQueue.java | 38 ++++++++-------- .../grizzly/ssl/SSLConnectionContext.java | 5 --- .../grizzly/http/io/OutputBuffer.java | 14 +++--- .../grizzly/http2/DefaultInputBuffer.java | 35 ++++++++------- .../grizzly/http2/Http2BaseFilter.java | 10 ++--- .../grizzly/http2/Http2FrameCodec.java | 14 ------ .../glassfish/grizzly/http2/Http2Session.java | 8 +--- .../grizzly/http2/Http2SessionOutputSink.java | 43 +++++++------------ .../glassfish/grizzly/http2/Http2Stream.java | 18 ++++---- 9 files changed, 75 insertions(+), 110 deletions(-) diff --git a/modules/grizzly/src/main/java/org/glassfish/grizzly/asyncqueue/TaskQueue.java b/modules/grizzly/src/main/java/org/glassfish/grizzly/asyncqueue/TaskQueue.java index 41c6d60545..f0092bc1be 100644 --- a/modules/grizzly/src/main/java/org/glassfish/grizzly/asyncqueue/TaskQueue.java +++ b/modules/grizzly/src/main/java/org/glassfish/grizzly/asyncqueue/TaskQueue.java @@ -39,17 +39,18 @@ public final class TaskQueue { */ private final Queue queue; - private static final AtomicReferenceFieldUpdater currentElementUpdater = AtomicReferenceFieldUpdater - .newUpdater(TaskQueue.class, AsyncQueueRecord.class, "currentElement"); + private static final AtomicReferenceFieldUpdater currentElementUpdater + = AtomicReferenceFieldUpdater.newUpdater(TaskQueue.class, AsyncQueueRecord.class, "currentElement"); private volatile E currentElement; - private static final AtomicIntegerFieldUpdater spaceInBytesUpdater = AtomicIntegerFieldUpdater.newUpdater(TaskQueue.class, "spaceInBytes"); + private static final AtomicIntegerFieldUpdater spaceInBytesUpdater + = AtomicIntegerFieldUpdater.newUpdater(TaskQueue.class, "spaceInBytes"); private volatile int spaceInBytes; private final MutableMaxQueueSize maxQueueSizeHolder; - private static final AtomicIntegerFieldUpdater writeHandlersCounterUpdater = AtomicIntegerFieldUpdater.newUpdater(TaskQueue.class, - "writeHandlersCounter"); + private static final AtomicIntegerFieldUpdater writeHandlersCounterUpdater + = AtomicIntegerFieldUpdater.newUpdater(TaskQueue.class, "writeHandlersCounter"); private volatile int writeHandlersCounter; protected final Queue writeHandlersQueue = new ConcurrentLinkedQueue<>(); // ------------------------------------------------------------ Constructors @@ -84,14 +85,15 @@ public int size() { @SuppressWarnings("unchecked") public E poll() { E current = (E) currentElementUpdater.getAndSet(this, null); - return current != null ? current : queue.poll(); + return current == null ? queue.poll() : current; } /** - * Get the current processing task, if the current in not set, take the task from the queue. Note: after this operation - * call, the current element could be removed from the queue using - * {@link #setCurrentElement(org.glassfish.grizzly.asyncqueue.AsyncQueueRecord)} and passing null as a - * parameter, this is a little bit more optimal alternative to {@link #poll()}. + * Get the current processing task, if the current in not set, take the task from the queue. + *

+ * Note: after this operation call, the current element could be removed from the queue using + * {@link #setCurrentElement(org.glassfish.grizzly.asyncqueue.AsyncQueueRecord)} + * and passing null as a parameter, this is a little bit more optimal alternative to {@link #poll()}. * * @return the current processing task */ @@ -155,7 +157,7 @@ public int spaceInBytes() { /** * Get the queue of tasks, which will be processed asynchronously - * + * * @return the queue of tasks, which will be processed asynchronously */ public Queue getQueue() { @@ -209,21 +211,21 @@ private void checkWriteHandlerOnClose(final WriteHandler writeHandler) { writeHandler.onError(new IOException("Connection is closed")); } } - // ------------------------------------------------------- Protected Methods + /** + * Notifies processing the queue by write handlers. + */ public void doNotify() { if (maxQueueSizeHolder == null || writeHandlersCounter == 0) { return; } final int maxQueueSize = maxQueueSizeHolder.getMaxQueueSize(); - while (spaceInBytes() < maxQueueSize) { - WriteHandler writeHandler = pollWriteHandler(); + final WriteHandler writeHandler = pollWriteHandler(); if (writeHandler == null) { return; } - try { writeHandler.onWritePossible(); } catch (Throwable e) { @@ -234,7 +236,7 @@ public void doNotify() { /** * Set current task element. - * + * * @param task current element. */ public void setCurrentElement(final E task) { @@ -260,7 +262,7 @@ public boolean compareAndSetCurrentElement(final E expected, final E newValue) { /** * Remove the task from queue. - * + * * @param task the task to remove. * @return true if tasked was removed, or false otherwise. */ @@ -336,8 +338,6 @@ private WriteHandler pollWriteHandler() { return null; } - // ----------------------------------------------------------- Nested Classes - public interface MutableMaxQueueSize { int getMaxQueueSize(); } diff --git a/modules/grizzly/src/main/java/org/glassfish/grizzly/ssl/SSLConnectionContext.java b/modules/grizzly/src/main/java/org/glassfish/grizzly/ssl/SSLConnectionContext.java index 10f9a06889..a92c40cde6 100644 --- a/modules/grizzly/src/main/java/org/glassfish/grizzly/ssl/SSLConnectionContext.java +++ b/modules/grizzly/src/main/java/org/glassfish/grizzly/ssl/SSLConnectionContext.java @@ -61,7 +61,6 @@ public final class SSLConnectionContext { } final ByteBufferArray outputByteBufferArray = ByteBufferArray.create(); - final ByteBufferArray inputByteBufferArray = ByteBufferArray.create(); private Buffer lastOutputBuffer; @@ -225,11 +224,9 @@ Buffer wrapAll(final Buffer input, final Allocator allocator) throws SSLExceptio if (input.hasRemaining()) { do { result = wrap(input, inputArray, inputArraySize, null, allocator); - if (result.isError()) { throw result.getError(); } - final Buffer newOutput = result.getOutput(); newOutput.trim(); @@ -272,13 +269,11 @@ private SslResult wrap(final Buffer input, final ByteBuffer[] inputArray, final } final Status status = sslEngineResult.getStatus(); - if (status == Status.CLOSED) { return new SslResult(output, new SSLException("SSLEngine is CLOSED")); } final boolean isOverflow = status == Status.BUFFER_OVERFLOW; - if (allocator != null && isOverflow) { updateBufferSizes(); output = ensureBufferSize(output, netBufferSize, allocator); diff --git a/modules/http/src/main/java/org/glassfish/grizzly/http/io/OutputBuffer.java b/modules/http/src/main/java/org/glassfish/grizzly/http/io/OutputBuffer.java index 1e30f5583c..c1acfee4e0 100644 --- a/modules/http/src/main/java/org/glassfish/grizzly/http/io/OutputBuffer.java +++ b/modules/http/src/main/java/org/glassfish/grizzly/http/io/OutputBuffer.java @@ -800,12 +800,11 @@ public void notifyCanWrite(final WriteHandler handler) { if (isNonBlockingWriteGuaranteed || canWrite()) { final Reentrant reentrant = Reentrant.getWriteReentrant(); - if (!reentrant.isMaxReentrantsReached()) { - notifyWritePossible(); - } else { + if (reentrant.isMaxReentrantsReached()) { notifyWritePossibleAsync(); + } else { + notifyWritePossible(); } - return; } @@ -847,7 +846,6 @@ protected Executor getThreadPool() { /** * Notify WriteHandler asynchronously */ - @SuppressWarnings("unchecked") private void notifyWritePossibleAsync() { if (writePossibleRunnable == null) { writePossibleRunnable = new Runnable() { @@ -1156,8 +1154,10 @@ private void flushBinaryBuffersIfNeeded() throws IOException { } private void notifyCommit() throws IOException { - for (int i = 0, len = lifeCycleListeners.size(); i < len; i++) { - lifeCycleListeners.get(i).onCommit(); + // the collection is not synchronized and may be accessed in parallel + final LifeCycleListener[] array = lifeCycleListeners.toArray(new LifeCycleListener[lifeCycleListeners.size()]); + for (LifeCycleListener lifeCycleListener : array) { + lifeCycleListener.onCommit(); } } diff --git a/modules/http2/src/main/java/org/glassfish/grizzly/http2/DefaultInputBuffer.java b/modules/http2/src/main/java/org/glassfish/grizzly/http2/DefaultInputBuffer.java index c87c448cd2..9e13cb78ef 100644 --- a/modules/http2/src/main/java/org/glassfish/grizzly/http2/DefaultInputBuffer.java +++ b/modules/http2/src/main/java/org/glassfish/grizzly/http2/DefaultInputBuffer.java @@ -47,9 +47,9 @@ class DefaultInputBuffer implements StreamInputBuffer { private static final long NULL_CONTENT_LENGTH = Long.MIN_VALUE; - private static final AtomicIntegerFieldUpdater inputQueueSizeUpdater = AtomicIntegerFieldUpdater.newUpdater(DefaultInputBuffer.class, - "inputQueueSize"); - @SuppressWarnings("unused") + private static final AtomicIntegerFieldUpdater inputQueueSizeUpdater + = AtomicIntegerFieldUpdater.newUpdater(DefaultInputBuffer.class, "inputQueueSize"); + private volatile int inputQueueSize; private final BlockingQueue inputQueue = new LinkedTransferQueue<>(); @@ -59,8 +59,8 @@ class DefaultInputBuffer implements StreamInputBuffer { // the termination flag. When is not null contains the reason why input was terminated. // when the flag is not null - poll0() will return -1. - private static final AtomicReferenceFieldUpdater closeFlagUpdater = AtomicReferenceFieldUpdater - .newUpdater(DefaultInputBuffer.class, Termination.class, "closeFlag"); + private static final AtomicReferenceFieldUpdater closeFlagUpdater + = AtomicReferenceFieldUpdater.newUpdater(DefaultInputBuffer.class, Termination.class, "closeFlag"); @SuppressWarnings("unused") private volatile Termination closeFlag; @@ -93,7 +93,6 @@ public void onReadEventComplete() { // If input stream has been terminated - send error message upstream if (isClosed()) { http2Session.sendMessageUpstream(stream, buildBrokenHttpContent(new EOFException(closeFlag.getDescription()))); - return; } @@ -262,17 +261,16 @@ private Buffer poll0() throws IOException { if (inputElement == null) { // timeout expired throw new IOException("Blocking read timeout"); - } else { - // Due to asynchronous inputQueueSize update - the inputQueueSizeNow may be < 0. - // It means the inputQueueSize.getAndSet(0); above, may unintentionally increase the counter. - // So, once we read a Buffer - we have to properly restore the counter value. - // Normally it had to be inputQueueSize.decrementAndGet(); , but we have to - // take into account fact described above. - inputQueueSizeUpdater.addAndGet(this, inputQueueSizeNow - 1); - - checkEOF(inputElement); - buffer = inputElement.toBuffer(); } + // Due to asynchronous inputQueueSize update - the inputQueueSizeNow may be < 0. + // It means the inputQueueSize.getAndSet(0); above, may unintentionally increase the counter. + // So, once we read a Buffer - we have to properly restore the counter value. + // Normally it had to be inputQueueSize.decrementAndGet(); , but we have to + // take into account fact described above. + inputQueueSizeUpdater.addAndGet(this, inputQueueSizeNow - 1); + + checkEOF(inputElement); + buffer = inputElement.toBuffer(); } else if (inputQueueSizeNow == 1) { // if there is one element available inputElement = inputQueue.poll(); @@ -375,14 +373,15 @@ public boolean isClosed() { /** * Checks if the passed InputElement is input buffer EOF element. - * + * * @param inputElement the {@link InputElement} to check EOF status against. */ private void checkEOF(final InputElement inputElement) { // first of all it has to be the last element if (inputElement.isLast) { - final Termination termination = !inputElement.isService ? IN_FIN_TERMINATION : (Termination) inputElement.content; + final Termination termination = inputElement.isService + ? (Termination) inputElement.content : IN_FIN_TERMINATION; if (closeFlagUpdater.compareAndSet(this, null, termination)) { diff --git a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2BaseFilter.java b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2BaseFilter.java index 66a0df917e..adcb93fbf5 100644 --- a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2BaseFilter.java +++ b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2BaseFilter.java @@ -119,7 +119,7 @@ public int getLocalMaxFramePayloadSize() { /** * Sets the maximum allowed HTTP2 frame size. - * + * * @param localMaxFramePayloadSize the maximum allowed HTTP2 frame size */ public void setLocalMaxFramePayloadSize(final int localMaxFramePayloadSize) { @@ -480,10 +480,10 @@ private void processSettingsFrame(final Http2Session http2Session, final FilterC } finally { frame.recycle(); } - } - void applySettings(final Http2Session http2Session, final SettingsFrame settingsFrame) throws Http2SessionException, Http2StreamException { + void applySettings(final Http2Session http2Session, final SettingsFrame settingsFrame) + throws Http2SessionException, Http2StreamException { for (int i = 0, numberOfSettings = settingsFrame.getNumberOfSettings(); i < numberOfSettings; i++) { final SettingsFrame.Setting setting = settingsFrame.getSettingByIndex(i); @@ -730,7 +730,7 @@ boolean checkIfHttp2StreamChain(final FilterChainContext ctx) throws IOException /** * Creates {@link Http2Session} with pre-configured initial-windows-size and max-concurrent-streams - * + * * @param connection the TCP {@link Connection} * @param isServer flag indicating whether this connection is server side or not. * @return {@link Http2Session} @@ -841,7 +841,7 @@ protected final Http2Session obtainHttp2Session(final FilterChainContext context final Http2Session obtainHttp2Session(final Http2State http2State, final FilterChainContext context, final boolean isUpStream) { final Connection connection = context.getConnection(); - Http2Session http2Session = http2State != null ? http2State.getHttp2Session() : null; + Http2Session http2Session = http2State == null ? null : http2State.getHttp2Session(); if (http2Session == null) { http2Session = Http2Session.get(connection); diff --git a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2FrameCodec.java b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2FrameCodec.java index a26903492d..23edab2003 100644 --- a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2FrameCodec.java +++ b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2FrameCodec.java @@ -71,20 +71,6 @@ public List parse(final Http2Session http2Session, final FrameParsin } return parsingResult.frameList(); - -// // ------------ ERROR processing block ----------------------------- -// final Buffer sndBuffer; -// final GoAwayFrame goAwayFrame = -// GoAwayFrame.builder() -// .errorCode(error.getErrorCode()) -// .build(); -// sndBuffer = goAwayFrame.toBuffer(http2State.getHttp2Session()); -// -// // send last message and close the connection -// ctx.write(sndBuffer); -// connection.closeSilently(); -// -// return ctx.getStopAction(); } public Buffer serializeAndRecycle(final Http2Session http2Session, final Http2Frame frame) { diff --git a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2Session.java b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2Session.java index 40a34b8f61..5388e49098 100644 --- a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2Session.java +++ b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2Session.java @@ -587,9 +587,7 @@ private void close() { @Override public void failed(final Throwable throwable) { - if (LOGGER.isLoggable(Level.FINE)) { - LOGGER.log(Level.FINE, "Unable to write GOAWAY. Terminating session.", throwable); - } + LOGGER.log(Level.WARNING, "Unable to write GOAWAY. Terminating session.", throwable); close(); } @@ -600,9 +598,7 @@ public void completed(final WriteResult result) { @Override public void cancelled() { - if (LOGGER.isLoggable(Level.FINE)) { - LOGGER.log(Level.FINE, "GOAWAY write cancelled. Terminating session."); - } + LOGGER.log(Level.FINE, "GOAWAY write cancelled. Terminating session."); close(); } }, null); diff --git a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2SessionOutputSink.java b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2SessionOutputSink.java index 086f2ad89d..62d866a7c9 100644 --- a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2SessionOutputSink.java +++ b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2SessionOutputSink.java @@ -48,7 +48,6 @@ public class Http2SessionOutputSink { protected final Http2Session http2Session; private static final Logger LOGGER = Grizzly.logger(Http2SessionOutputSink.class); - private static final Level LOGGER_LEVEL = Level.FINE; private static final int MAX_FRAME_PAYLOAD_SIZE = 16383; private static final int MAX_OUTPUT_QUEUE_SIZE = 65536; @@ -127,10 +126,9 @@ protected void onPeerWindowUpdate(final int delta) throws Http2SessionException throw new Http2SessionException(ErrorCode.FLOW_CONTROL_ERROR, "Session flow-control window overflow."); } final int newWindowSize = availConnectionWindowSize.addAndGet(delta); - if (LOGGER.isLoggable(LOGGER_LEVEL)) { - LOGGER.log(LOGGER_LEVEL, "Http2Session. Expand connection window size by {0} bytes. Current connection window size is: {1}", - new Object[] { delta, newWindowSize }); - } + LOGGER.log(Level.FINE, + "Http2Session. Expand connection window size by {0} bytes. Current connection window size is: {1}", + new Object[] {delta, newWindowSize}); flushOutputQueue(); } @@ -171,7 +169,6 @@ protected void writeDataDownStream(final Http2Stream stream, final List writeCompletionHandler = null; int writeCompletionHandlerBytes = 0; - int bytesToTransfer = 0; int queueSizeToFree = 0; - AggrCompletionHandler completionHandlers = null; - // gather all available output data frames while (availWindowSize > bytesToTransfer && queueSize > queueSizeToFree) { final Http2OutputQueueRecord record = outputQueue.poll(); - if (record == null) { - // keep this warning for now - // should be reported when null record is spotted - LOGGER.log(Level.WARNING, "UNEXPECTED NULL RECORD. Queue-size: {0} " + "tmpcnt={1} byteToTransfer={2} queueSizeToFree={3} queueSize={4}", - new Object[] { outputQueue.size(), tmpcnt, bytesToTransfer, queueSizeToFree, queueSize }); + // keep this warning for now - should be reported when null record is spotted + LOGGER.log(Level.WARNING, "UNEXPECTED NULL RECORD. Queue-size: {0} " + + "byteToTransfer={1} queueSizeToFree={2} queueSize={3}", + new Object[]{outputQueue.size(), bytesToTransfer, queueSizeToFree, queueSize}); } assert record != null; - final int serializedBytes = record.serializeTo(tmpFramesList, Math.min(MAX_FRAME_PAYLOAD_SIZE, availWindowSize - bytesToTransfer)); + final int serializedBytes = record.serializeTo(tmpFramesList, + Math.min(MAX_FRAME_PAYLOAD_SIZE, availWindowSize - bytesToTransfer)); bytesToTransfer += serializedBytes; queueSizeToFree += serializedBytes; @@ -264,12 +256,10 @@ assert record != null; outputQueue.releaseSpace(queueSizeToFree); - needToNotify = true; - if (LOGGER.isLoggable(LOGGER_LEVEL)) { - LOGGER.log(LOGGER_LEVEL, "Http2Session. Shrink connection window size by {0} bytes. Current connection window size is: {1}", - new Object[] { bytesToTransfer, newWindowSize }); - } - + needToNotifyQueueManagement = true; + LOGGER.log(Level.FINE, + "Http2Session. Shrink connection window size by {0} bytes. Current connection window size is: {1}", + new Object[] {bytesToTransfer, newWindowSize}); } // release the writer lock, so other thread can start to write @@ -278,10 +268,9 @@ assert record != null; // we don't want this thread to write all the time - so give more // time for another thread to start writing LockSupport.parkNanos(backoffDelay++); - tmpcnt++; } - if (needToNotify) { + if (needToNotifyQueueManagement) { outputQueue.doNotify(); } } diff --git a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2Stream.java b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2Stream.java index bc650fe6e3..ac8803e27c 100644 --- a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2Stream.java +++ b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2Stream.java @@ -75,8 +75,8 @@ public enum State { static final int UPGRADE_STREAM_ID = 1; - private static final Attribute HTTP_RQST_HTTP2_STREAM_ATTR = AttributeBuilder.DEFAULT_ATTRIBUTE_BUILDER - .createAttribute("http2.request.stream"); + private static final Attribute HTTP_RQST_HTTP2_STREAM_ATTR + = AttributeBuilder.DEFAULT_ATTRIBUTE_BUILDER.createAttribute("http2.request.stream"); State state = State.IDLE; @@ -94,13 +94,14 @@ public enum State { final StreamOutputSink outputSink; // number of bytes reported to be read, but still unacked to the peer - static final AtomicIntegerFieldUpdater unackedReadBytesUpdater = AtomicIntegerFieldUpdater.newUpdater(Http2Stream.class, "unackedReadBytes"); + static final AtomicIntegerFieldUpdater unackedReadBytesUpdater + = AtomicIntegerFieldUpdater.newUpdater(Http2Stream.class, "unackedReadBytes"); @SuppressWarnings("unused") private volatile int unackedReadBytes; // closeReasonRef, "null" value means the connection is open. - private static final AtomicReferenceFieldUpdater closeReasonUpdater = AtomicReferenceFieldUpdater.newUpdater(Http2Stream.class, - CloseReason.class, "closeReason"); + private static final AtomicReferenceFieldUpdater closeReasonUpdater + = AtomicReferenceFieldUpdater.newUpdater(Http2Stream.class, CloseReason.class, "closeReason"); @SuppressWarnings("unused") private volatile CloseReason closeReason; @@ -108,8 +109,8 @@ public enum State { private final Queue closeListeners = new ConcurrentLinkedQueue<>(); - private static final AtomicIntegerFieldUpdater completeFinalizationCounterUpdater = AtomicIntegerFieldUpdater.newUpdater(Http2Stream.class, - "completeFinalizationCounter"); + private static final AtomicIntegerFieldUpdater completeFinalizationCounterUpdater + = AtomicIntegerFieldUpdater.newUpdater(Http2Stream.class, "completeFinalizationCounter"); @SuppressWarnings("unused") private volatile int completeFinalizationCounter; @@ -179,7 +180,6 @@ protected Http2Stream(final Http2Session http2Session, final HttpRequestPacket r this.exclusive = false; inputBuffer = http2Session.isServer() ? new UpgradeInputBuffer(this) : new DefaultInputBuffer(this); - outputSink = http2Session.isServer() ? new DefaultOutputSink(this) : new UpgradeOutputSink(http2Session); HTTP_RQST_HTTP2_STREAM_ATTR.set(request, this); @@ -318,7 +318,7 @@ public void closeSilently() { /** * {@inheritDoc} - * + * * @deprecated please use {@link #close()} with the following * {@link GrizzlyFuture#addCompletionHandler(org.glassfish.grizzly.CompletionHandler)} call */ From 8986bb85586694c42bb0f8733d56915f21b80beb Mon Sep 17 00:00:00 2001 From: David Matejcek Date: Thu, 19 Nov 2020 23:41:19 +0100 Subject: [PATCH 17/26] Issue #2016 Fixed maximum payload error. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Matthew Gill Signed-off-by: David Matějček --- .../main/java/org/glassfish/grizzly/http2/Http2FrameCodec.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2FrameCodec.java b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2FrameCodec.java index 23edab2003..9d6875fcef 100644 --- a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2FrameCodec.java +++ b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2FrameCodec.java @@ -115,7 +115,7 @@ private ParsingResult parseFrame(final Http2Session http2Session, final FramePar final int len = http2Session.getFrameSize(buffer); - if (len > http2Session.getLocalMaxFramePayloadSize() + Http2Frame.FRAME_HEADER_SIZE) { + if (len > http2Session.getPeerMaxFramePayloadSize() + Http2Frame.FRAME_HEADER_SIZE) { http2Session.onOversizedFrame(buffer); From 2c531b46d3d7cf378bbc2bb42cbcf8fe00efad31 Mon Sep 17 00:00:00 2001 From: David Matejcek Date: Thu, 19 Nov 2020 23:43:31 +0100 Subject: [PATCH 18/26] Issue #2016 Fixed NPE on missing CloseReason MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - original code was throwing NPE - handler can be null - then we have nothing to handle the reason - closeReason can be null - then we use "locally closed" as a default (may be incorrect) - even ignored exception should be visible somewhere (logging) Signed-off-by: David Matějček --- .../org/glassfish/grizzly/http/io/OutputBuffer.java | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/modules/http/src/main/java/org/glassfish/grizzly/http/io/OutputBuffer.java b/modules/http/src/main/java/org/glassfish/grizzly/http/io/OutputBuffer.java index c1acfee4e0..60ee26ea49 100644 --- a/modules/http/src/main/java/org/glassfish/grizzly/http/io/OutputBuffer.java +++ b/modules/http/src/main/java/org/glassfish/grizzly/http/io/OutputBuffer.java @@ -40,6 +40,7 @@ import java.util.logging.Logger; import org.glassfish.grizzly.Buffer; +import org.glassfish.grizzly.CloseReason; import org.glassfish.grizzly.CompletionHandler; import org.glassfish.grizzly.Connection; import org.glassfish.grizzly.FileTransfer; @@ -791,8 +792,14 @@ public void notifyCanWrite(final WriteHandler handler) { throw new IllegalStateException("Illegal attempt to set a new handler before the existing handler has been notified."); } - if (!httpContext.getCloseable().isOpen()) { - handler.onError(connection.getCloseReason().getCause()); + if (handler != null && !httpContext.getCloseable().isOpen()) { + final CloseReason closeReason = connection.getCloseReason(); + if (closeReason == null) { + LOGGER.log(Level.WARNING, "No close reason set, using default: {0}", CloseReason.LOCALLY_CLOSED_REASON); + handler.onError(CloseReason.LOCALLY_CLOSED_REASON.getCause()); + } else { + handler.onError(closeReason.getCause()); + } return; } @@ -820,6 +827,7 @@ public void notifyCanWrite(final WriteHandler handler) { // have been processed by WriteHandler.onError(). httpContext.getOutputSink().notifyCanWrite(asyncWriteHandler); } catch (Exception ignored) { + LOGGER.log(Level.FINE, "Ignoring exception.", ignored); } } From 488d5a65f8f742479ba9f1f57ada34f5acb1340d Mon Sep 17 00:00:00 2001 From: David Matejcek Date: Thu, 19 Nov 2020 23:45:32 +0100 Subject: [PATCH 19/26] Issue #2016 Fixed IllegalStateException MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - based on h2spec results - client was able to cause ISE on the server simply by misformatted header - assertions are worthless in production Signed-off-by: David Matějček --- .../glassfish/grizzly/http2/Http2Session.java | 4 +++- .../org/glassfish/grizzly/http2/Http2Stream.java | 16 ++++++++++------ 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2Session.java b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2Session.java index 5388e49098..f9cfdebe44 100644 --- a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2Session.java +++ b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2Session.java @@ -280,7 +280,9 @@ protected int getFrameSize(final Buffer buffer) { public Http2Frame parseHttp2FrameHeader(final Buffer buffer) throws Http2SessionException { // we assume the passed buffer represents only this frame, no remainders allowed final int len = getFrameSize(buffer); - assert buffer.remaining() == len; + if (buffer.remaining() != len) { + throw new Http2SessionException(ErrorCode.FRAME_SIZE_ERROR); + } final int i1 = buffer.getInt(); diff --git a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2Stream.java b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2Stream.java index ac8803e27c..3cdccbe0f0 100644 --- a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2Stream.java +++ b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2Stream.java @@ -596,7 +596,7 @@ void offerInputData(final Buffer data, final boolean fin) throws IOException { } } - void flushInputData() { + void flushInputData() throws Http2SessionException { final Buffer cachedInputBufferLocal = cachedInputBuffer; final boolean cachedIsLastLocal = cachedIsLast; cachedInputBuffer = null; @@ -614,11 +614,15 @@ void flushInputData() { } final int size = cachedInputBufferLocal.remaining(); - if (!inputBuffer.offer(cachedInputBufferLocal, cachedIsLastLocal)) { - // if we can't add this buffer to the stream input buffer - - // we have to release the part of connection window allocated - // for the buffer - http2Session.ackConsumedData(size); + try { + if (!inputBuffer.offer(cachedInputBufferLocal, cachedIsLastLocal)) { + // if we can't add this buffer to the stream input buffer - + // we have to release the part of connection window allocated + // for the buffer + http2Session.ackConsumedData(size); + } + } catch (final RuntimeException e) { + throw new Http2SessionException(ErrorCode.PROTOCOL_ERROR, e.getMessage()); } } } From 716b945803025f08dc94e2947941bde940a1f98f Mon Sep 17 00:00:00 2001 From: David Matejcek Date: Thu, 19 Nov 2020 22:48:50 +0100 Subject: [PATCH 20/26] Issue #2016 Escape both while loops if the record is null MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - fixes another NPE seen in logs Signed-off-by: David Matějček --- .../glassfish/grizzly/http2/Http2SessionOutputSink.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2SessionOutputSink.java b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2SessionOutputSink.java index 62d866a7c9..f0421d7f6c 100644 --- a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2SessionOutputSink.java +++ b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2SessionOutputSink.java @@ -198,6 +198,7 @@ private void flushOutputQueue() { int writeCompletionHandlerBytes = 0; int bytesToTransfer = 0; int queueSizeToFree = 0; + boolean breakNow = false; // gather all available output data frames while (availWindowSize > bytesToTransfer && queueSize > queueSizeToFree) { @@ -208,10 +209,9 @@ private void flushOutputQueue() { LOGGER.log(Level.WARNING, "UNEXPECTED NULL RECORD. Queue-size: {0} " + "byteToTransfer={1} queueSizeToFree={2} queueSize={3}", new Object[]{outputQueue.size(), bytesToTransfer, queueSizeToFree, queueSize}); + breakNow = true; + break; } - - assert record != null; - final int serializedBytes = record.serializeTo(tmpFramesList, Math.min(MAX_FRAME_PAYLOAD_SIZE, availWindowSize - bytesToTransfer)); bytesToTransfer += serializedBytes; @@ -264,6 +264,9 @@ assert record != null; // release the writer lock, so other thread can start to write writerLock.set(false); + if (breakNow) { + break; + } // we don't want this thread to write all the time - so give more // time for another thread to start writing From 5b157a466d649d56f9f6f7961ea9df3ea5e41063 Mon Sep 17 00:00:00 2001 From: David Matejcek Date: Thu, 19 Nov 2020 22:53:27 +0100 Subject: [PATCH 21/26] Issue #2016 Added logging for pruning streams MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: David Matějček --- .../src/main/java/org/glassfish/grizzly/http2/Http2Session.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2Session.java b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2Session.java index f9cfdebe44..737321fa23 100644 --- a/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2Session.java +++ b/modules/http2/src/main/java/org/glassfish/grizzly/http2/Http2Session.java @@ -634,6 +634,7 @@ private GoAwayFrame setGoAwayLocally(final ErrorCode errorCode, final String det // Must be locked by sessionLock private void pruneStreams() { + LOGGER.log(Level.FINE, "pruneStreams()"); // close streams that rank above the last stream ID specified by the GOAWAY frame. // Allow other streams to continue processing. Once the concurrent stream count reaches zero, // the session will be closed. @@ -1060,6 +1061,7 @@ FilterChain getHttp2SessionChain() { * Called from {@link Http2Stream} once stream is completely closed. */ void deregisterStream() { + LOGGER.fine("deregisterStream()"); final boolean isCloseSession; synchronized (sessionLock) { decStreamCount(); From 360f4318d106ec24a9626252e902dba3c256beaa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mat=C4=9Bj=C4=8Dek?= Date: Fri, 20 Nov 2020 15:45:26 +0100 Subject: [PATCH 22/26] Issue #2016 ALPN support test reenabled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Test results: - OpenJDK 8u275, openjsse 1.1.5 ... SUCCESS - OpenJDK 11.0.8, openjsse 1.1.5 ... FAIL ... as expected - OpenJDK 8u275, npn bootstrap 1.9 ... FAIL ... as expected - OpenJDK 8u232, npn bootstrap 1.9.payara-p1 ... SUCCESS - OpenJDK 8u275 ... SUCCESS - OpenJDK 11.0.8 ... SUCCESS Signed-off-by: David Matějček --- .../test/java/org/glassfish/grizzly/http2/AlpnSupportTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/modules/http2/src/test/java/org/glassfish/grizzly/http2/AlpnSupportTest.java b/modules/http2/src/test/java/org/glassfish/grizzly/http2/AlpnSupportTest.java index 98066291e0..80c9ac2431 100644 --- a/modules/http2/src/test/java/org/glassfish/grizzly/http2/AlpnSupportTest.java +++ b/modules/http2/src/test/java/org/glassfish/grizzly/http2/AlpnSupportTest.java @@ -17,6 +17,7 @@ import org.glassfish.grizzly.ssl.SSLUtils; import org.junit.After; import org.junit.Before; +import org.junit.Test; public class AlpnSupportTest extends AbstractHttp2Test { @@ -42,7 +43,7 @@ public void destroy() throws IOException { /** * Tests that the SSL_TO_CONNECTION_MAP removes entries from it after the connection closes. */ - // @Test + @Test public void sslToConnectionMapClearTest() throws Exception { SSLEngine engine = null; From 9b2245780a559d28c70eeb3c8b120f109ca7166a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mat=C4=9Bj=C4=8Dek?= Date: Fri, 20 Nov 2020 16:03:57 +0100 Subject: [PATCH 23/26] Updated travis file Signed-off-by: David Matejcek --- .travis.yml | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/.travis.yml b/.travis.yml index b3872647ae..8cb5b757bb 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,11 +1,9 @@ language: java -dist: trusty +dist: bionic jdk: - - oraclejdk8 + - openjdk8 before_install: - - wget https://downloads.apache.org/maven/maven-3/3.6.3/binaries/apache-maven-3.6.3-bin.tar.gz - - tar xzvf apache-maven-3.6.3-bin.tar.gz - - export PATH=`pwd`/apache-maven-3.6.3/bin:$PATH - mvn -v sudo: false script: mvn --fail-at-end clean verify + From 28635f35983388ad7f56faa14e77f8195a9f3305 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mat=C4=9Bj=C4=8Dek?= Date: Sat, 21 Nov 2020 13:13:57 +0100 Subject: [PATCH 24/26] Issue #2016 Fixed dependency version of NPN Signed-off-by: David Matejcek --- pom.xml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pom.xml b/pom.xml index f651bef7ff..712795b9d2 100644 --- a/pom.xml +++ b/pom.xml @@ -125,8 +125,8 @@ 4.2.1 2.4 4.0.0 - 2.0 - 2.0 + 2.0.0 + 2.0.0 1.1.5 4.2.0 From b5f23d1d0d55eb53129e1e0b47e86e549d169fad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mat=C4=9Bj=C4=8Dek?= Date: Fri, 20 Nov 2020 19:10:13 +0100 Subject: [PATCH 25/26] Using current Ubuntu Focal (20.04) and openjdk8, fixed warnings Signed-off-by: David Matejcek --- .travis.yml | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/.travis.yml b/.travis.yml index 8cb5b757bb..1f1114d981 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,9 +1,15 @@ language: java -dist: bionic -jdk: - - openjdk8 +os: linux +dist: focal +addons: + apt: + packages: + - openjdk-8-jdk +env: + global: + - PATH=/usr/lib/jvm/java-8-openjdk-amd64/bin/:$PATH + - JAVA_HOME=/usr/lib/jvm/java-8-openjdk-amd64 before_install: - mvn -v -sudo: false script: mvn --fail-at-end clean verify From 87569a59d540820869e04b05992f611ba8255453 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mat=C4=9Bj=C4=8Dek?= Date: Sat, 21 Nov 2020 13:28:21 +0100 Subject: [PATCH 26/26] Enabled staging repository - required to be able to access staging repository with grizzly npn on Travis - discussed with Arjan - artifacts are not cached on Travis, so it should not cause any harm Signed-off-by: David Matejcek --- .travis.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 1f1114d981..ea5e6899b6 100644 --- a/.travis.yml +++ b/.travis.yml @@ -11,5 +11,7 @@ env: - JAVA_HOME=/usr/lib/jvm/java-8-openjdk-amd64 before_install: - mvn -v -script: mvn --fail-at-end clean verify +install: + - mvn install -DskipTests=true -Pstaging -Dmaven.javadoc.skip=true -B -V +script: mvn --fail-at-end clean verify -Pstaging