From df23c52f398ddbb36854aec6c2ae1cf9bb1b3dc9 Mon Sep 17 00:00:00 2001 From: Matt Gill Date: Thu, 16 Aug 2018 12:32:41 +0100 Subject: [PATCH 1/4] Fixed maximum payload error. --- .../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 7ef0810ef9..07c8e81c52 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 @@ -139,7 +139,7 @@ private ParsingResult parseFrame(final Http2Session http2Session, 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 a682c3baaee37034daa65eff187ad19b205c658f Mon Sep 17 00:00:00 2001 From: Matt Gill Date: Mon, 20 Aug 2018 11:50:19 +0100 Subject: [PATCH 2/4] Added trailer to output queue to prevent early connection closure. --- .../grizzly/http2/DefaultOutputSink.java | 112 +++++++++++++----- 1 file changed, 81 insertions(+), 31 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 fec49bf64f..2990764dae 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 @@ -16,13 +16,16 @@ package org.glassfish.grizzly.http2; +import static java.util.logging.Level.FINE; +import static java.util.logging.Level.WARNING; + import java.io.IOException; import java.util.ArrayList; import java.util.HashMap; 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; import org.glassfish.grizzly.CompletionHandler; @@ -117,10 +120,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()}); - } + LOGGER.log(FINE, "Terminated!!! id={0} description={1}", + new Object[]{stream.getId(), terminationFlag.getDescription()}); throw new IOException(terminationFlag.getDescription()); } else if (isLastFrameQueued) { throw new IOException("Write beyond end of stream"); @@ -148,7 +149,7 @@ public void onPeerWindowUpdate(final int delta) throws Http2StreamException { // try to write until window limit allows while (isWantToWrite() && - !outputQueue.isEmpty()) { + !outputQueue.isEmpty()) { // pick up the first output record in the queue OutputQueueRecord outputQueueRecord = outputQueue.poll(); @@ -171,6 +172,18 @@ public void onPeerWindowUpdate(final int delta) throws Http2StreamException { boolean isLast = outputQueueRecord.isLast; 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 @@ -178,7 +191,6 @@ public void onPeerWindowUpdate(final int delta) throws Http2StreamException { final Buffer dataChunkToSend = resource.read(bytesToSend); final boolean hasRemaining = resource.hasRemaining(); - // if there is a chunk to store if (hasRemaining) { // Create output record for the chunk to be stored @@ -219,6 +231,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; + } } /** @@ -251,8 +278,8 @@ public synchronized void writeDownStream(final HttpPacket httpPacket, List headerFrames = null; OutputQueueRecord outputQueueRecord = null; - boolean isDeflaterLocked = false; - + final ReentrantLock deflatorLock = http2Session.getDeflaterLock(); + try { // try-finally block to release deflater lock if needed // If HTTP header hasn't been committed - commit it @@ -263,8 +290,7 @@ public synchronized void writeDownStream(final HttpPacket httpPacket, !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( @@ -319,15 +345,6 @@ public synchronized void writeDownStream(final HttpPacket httpPacket, 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 = @@ -349,10 +366,19 @@ public synchronized void writeDownStream(final HttpPacket httpPacket, 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); @@ -367,7 +393,6 @@ public synchronized void writeDownStream(final HttpPacket httpPacket, } // our element is first in the output queue - final int remaining = data.remaining(); // check if output record's buffer is fitting into window size @@ -428,7 +453,6 @@ public synchronized void writeDownStream(final HttpPacket httpPacket, if (isLast) { if (isTrailer) { // !!!!! LOCK the deflater - isDeflaterLocked = true; sendTrailers(completionHandler, messageCloner, (HttpTrailer) httpContent); } close(); @@ -436,8 +460,8 @@ public synchronized void writeDownStream(final HttpPacket httpPacket, } } finally { - if (isDeflaterLocked) { - http2Session.getDeflaterLock().unlock(); + if (deflatorLock.isHeldByCurrentThread()) { + deflatorLock.unlock(); } } @@ -603,7 +627,7 @@ private void addOutputQueueRecord(OutputQueueRecord outputQueueRecord) throws Http2StreamException { do { // Make sure current outputQueueRecord is not forgotten - + // set the outputQueueRecord as the current outputQueue.setCurrentElement(outputQueueRecord); @@ -612,10 +636,20 @@ private void addOutputQueueRecord(OutputQueueRecord outputQueueRecord) outputQueue.compareAndSetCurrentElement(outputQueueRecord, null)) { // 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; @@ -624,7 +658,6 @@ private void addOutputQueueRecord(OutputQueueRecord outputQueueRecord) 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 @@ -707,11 +740,16 @@ private void sendTrailers(final CompletionHandler completionHandler 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; @@ -727,6 +765,18 @@ public OutputQueueRecord(final Source resource, this.isLast = isLast; 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) { From 0a59fd66292976a2a7af97df62ec87ae117feff6 Mon Sep 17 00:00:00 2001 From: Matt Gill Date: Mon, 20 Aug 2018 12:55:12 +0100 Subject: [PATCH 3/4] Removed second check for trailer. --- .../grizzly/http2/DefaultOutputSink.java | 22 +++++-------------- 1 file changed, 6 insertions(+), 16 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 2990764dae..56c42df425 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 @@ -146,10 +146,10 @@ public void onPeerWindowUpdate(final int delta) throws Http2StreamException { } // update the available window size 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(); @@ -178,6 +178,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); @@ -233,18 +234,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."); } } @@ -372,7 +362,7 @@ public synchronized void writeDownStream(final HttpPacket httpPacket, .createBufferSource(data), flushCompletionHandler, (HttpTrailer) httpContent, - isZeroSizeData); + false); } else { outputQueueRecord = new OutputQueueRecord( Source.factory(stream) From 309531dfd506c3bba707bb51cc2f6c66e1065eb8 Mon Sep 17 00:00:00 2001 From: Matt Gill Date: Mon, 20 Aug 2018 13:30:35 +0100 Subject: [PATCH 4/4] Removed second deflator lock. --- .../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 56c42df425..7fe34e9a80 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 @@ -232,10 +232,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."); - } } /** @@ -445,7 +441,6 @@ public synchronized void writeDownStream(final HttpPacket httpPacket, // !!!!! LOCK the deflater sendTrailers(completionHandler, messageCloner, (HttpTrailer) httpContent); } - close(); return; } @@ -711,7 +706,6 @@ 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 = @@ -730,7 +724,6 @@ private void sendTrailers(final CompletionHandler completionHandler flushToConnectionOutputSink(trailerFrames, null, new FlushCompletionHandler(completionHandler), messageCloner, true); - http2Session.getDeflaterLock().unlock(); close(); }