Improve javadoc and exception handling for http3-qpack

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/Instruction.java

@@ -21,6 +21,12 @@ public interface Instruction
 {
     void encode(ByteBufferPool.Lease lease);
 
+    /**
+     * <p>A handler for instructions issued by an {@link QpackEncoder} or {@link QpackDecoder}.</p>
+     * <p>Note: an encoder SHOULD NOT write an instruction unless sufficient stream and connection flow control
+     * credit is available for the entire instruction, otherwise a stream containing a large instruction can become
+     * deadlocked.</p>
+     */
     interface Handler
     {
         void onInstructions(List<Instruction> instructions);

jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/QpackDecoder.java

@@ -28,15 +28,14 @@ import org.eclipse.jetty.http3.qpack.internal.parser.EncodedFieldSection;
 import org.eclipse.jetty.http3.qpack.internal.table.DynamicTable;
 import org.eclipse.jetty.http3.qpack.internal.table.Entry;
 import org.eclipse.jetty.http3.qpack.internal.table.StaticTable;
-import org.eclipse.jetty.http3.qpack.internal.util.EncodingException;
 import org.eclipse.jetty.http3.qpack.internal.util.NBitIntegerParser;
 import org.eclipse.jetty.util.BufferUtil;
 import org.eclipse.jetty.util.component.Dumpable;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import static org.eclipse.jetty.http3.qpack.QpackException.QPACK_DECODER_STREAM_ERROR;
 import static org.eclipse.jetty.http3.qpack.QpackException.QPACK_DECOMPRESSION_FAILED;
+import static org.eclipse.jetty.http3.qpack.QpackException.QPACK_ENCODER_STREAM_ERROR;
 
 public class QpackDecoder implements Dumpable
 {
@@ -77,7 +76,7 @@ public class QpackDecoder implements Dumpable
     {
         _context = new QpackContext();
         _handler = handler;
-        _parser = new DecoderInstructionParser(new DecoderAdapter());
+        _parser = new DecoderInstructionParser(new InstructionHandler());
         _maxHeaderSize = maxHeaderSize;
     }
 
@@ -150,18 +149,20 @@ public class QpackDecoder implements Dumpable
                     LOG.debug("Deferred Decoding: streamId={}, encodedFieldSection={}", streamId, encodedFieldSection);
                 _encodedFieldSections.add(encodedFieldSection);
             }
+
+            boolean hadMetaData = !_metaDataNotifications.isEmpty();
+            notifyInstructionHandler();
+            notifyMetaDataHandler();
+            return hadMetaData;
+        }
+        catch (QpackException.SessionException e)
+        {
+            throw e;
         }
         catch (Throwable t)
         {
-            notifyInstructionHandler();
+            throw new QpackException.SessionException(QPACK_ENCODER_STREAM_ERROR, t.getMessage(), t);
-            notifyMetaDataHandler();
-            throw t;
         }
-
-        boolean hadMetaData = !_metaDataNotifications.isEmpty();
-        notifyInstructionHandler();
-        notifyMetaDataHandler();
-        return hadMetaData;
     }
 
     /**
@@ -179,17 +180,17 @@ public class QpackDecoder implements Dumpable
             {
                 _parser.parse(buffer);
             }
-        }
-        catch (EncodingException e)
-        {
-            // There was an error decoding the instruction.
-            throw new QpackException.SessionException(QPACK_DECODER_STREAM_ERROR, e.getMessage(), e);
-        }
-        finally
-        {
             notifyInstructionHandler();
             notifyMetaDataHandler();
         }
+        catch (QpackException.SessionException e)
+        {
+            throw e;
+        }
+        catch (Throwable t)
+        {
+            throw new QpackException.SessionException(QPACK_ENCODER_STREAM_ERROR, t.getMessage(), t);
+        }
     }
 
     private void checkEncodedFieldSections() throws QpackException
@@ -302,7 +303,7 @@ public class QpackDecoder implements Dumpable
         return String.format("QpackDecoder@%x{%s}", hashCode(), _context);
     }
 
-    private void notifyInstructionHandler() throws QpackException
+    private void notifyInstructionHandler()
     {
         if (!_instructions.isEmpty())
             _handler.onInstructions(_instructions);
@@ -321,7 +322,7 @@ public class QpackDecoder implements Dumpable
     /**
      * This delivers notifications from the DecoderInstruction parser directly into the Decoder.
      */
-    class DecoderAdapter implements DecoderInstructionParser.Handler
+    class InstructionHandler implements DecoderInstructionParser.Handler
     {
         @Override
         public void onSetDynamicTableCapacity(int capacity)

jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/QpackEncoder.java

@@ -14,6 +14,7 @@
 package org.eclipse.jetty.http3.qpack;
 
 import java.io.IOException;
+import java.nio.BufferOverflowException;
 import java.nio.ByteBuffer;
 import java.util.ArrayList;
 import java.util.EnumSet;
@@ -43,6 +44,7 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import static org.eclipse.jetty.http3.qpack.QpackException.H3_GENERAL_PROTOCOL_ERROR;
+import static org.eclipse.jetty.http3.qpack.QpackException.QPACK_DECODER_STREAM_ERROR;
 import static org.eclipse.jetty.http3.qpack.QpackException.QPACK_ENCODER_STREAM_ERROR;
 
 public class QpackEncoder implements Dumpable
@@ -98,7 +100,7 @@ public class QpackEncoder implements Dumpable
         _handler = handler;
         _context = new QpackContext();
         _maxBlockedStreams = maxBlockedStreams;
-        _parser = new EncoderInstructionParser(new EncoderAdapter());
+        _parser = new EncoderInstructionParser(new InstructionHandler());
     }
 
     /**
@@ -140,6 +142,10 @@ public class QpackEncoder implements Dumpable
         List<EncodableEntry> encodableEntries = new ArrayList<>();
         DynamicTable dynamicTable = _context.getDynamicTable();
 
+        // We need to remember what fields were referenced for each stream for multiple reasons:
+        //  1. We can only (potentially) block up to SETTINGS_QPACK_BLOCKED_STREAMS by referencing entries which may not have arrived.
+        //  2. We need to remember reference counts to each entry which are then acknowledged by the remote decoder, this
+        //     allows us to know when we can evict an entry (when it has no un-acknowledged references).
         StreamInfo streamInfo = _streamInfoMap.get(streamId);
         if (streamInfo == null)
         {
@@ -149,41 +155,57 @@ public class QpackEncoder implements Dumpable
         StreamInfo.SectionInfo sectionInfo = new StreamInfo.SectionInfo();
         streamInfo.add(sectionInfo);
 
-        int requiredInsertCount = 0;
+        try
-
-        // This will also extract pseudo headers from the metadata.
-        Http3Fields httpFields = new Http3Fields(metadata);
-        for (HttpField field : httpFields)
         {
-            EncodableEntry entry = encode(streamInfo, field);
+            int requiredInsertCount = 0;
-            encodableEntries.add(entry);
+            for (HttpField field : new Http3Fields(metadata))
+            {
+                EncodableEntry entry = encode(streamInfo, field);
+                encodableEntries.add(entry);
 
-            // Update the required InsertCount.
+                // Update the required InsertCount.
-            int entryRequiredInsertCount = entry.getRequiredInsertCount();
+                int entryRequiredInsertCount = entry.getRequiredInsertCount();
-            if (entryRequiredInsertCount > requiredInsertCount)
+                if (entryRequiredInsertCount > requiredInsertCount)
-                requiredInsertCount = entryRequiredInsertCount;
+                    requiredInsertCount = entryRequiredInsertCount;
+            }
+
+            sectionInfo.setRequiredInsertCount(requiredInsertCount);
+            int base = dynamicTable.getBase();
+            int encodedInsertCount = encodeInsertCount(requiredInsertCount, dynamicTable.getCapacity());
+            boolean signBit = base < requiredInsertCount;
+            int deltaBase = signBit ? requiredInsertCount - base - 1 : base - requiredInsertCount;
+
+            // Encode all the entries into the buffer.
+            int pos = BufferUtil.flipToFill(buffer);
+
+            // Encode the Field Section Prefix into the ByteBuffer.
+            NBitIntegerEncoder.encode(buffer, 8, encodedInsertCount);
+            buffer.put(signBit ? (byte)0x80 : (byte)0x00);
+            NBitIntegerEncoder.encode(buffer, 7, deltaBase);
+
+            // Encode the field lines into the ByteBuffer.
+            for (EncodableEntry entry : encodableEntries)
+            {
+                entry.encode(buffer, base);
+            }
+
+            BufferUtil.flipToFlush(buffer, pos);
+            notifyInstructionHandler();
         }
-
+        catch (BufferOverflowException e)
-        sectionInfo.setRequiredInsertCount(requiredInsertCount);
-        int base = dynamicTable.getBase();
-        int encodedInsertCount = encodeInsertCount(requiredInsertCount, dynamicTable.getCapacity());
-        boolean signBit = base < requiredInsertCount;
-        int deltaBase = signBit ? requiredInsertCount - base - 1 : base - requiredInsertCount;
-
-        // Encode all the entries into the buffer.
-
-        // Encode the Field Section Prefix into the ByteBuffer.
-        NBitIntegerEncoder.encode(buffer, 8, encodedInsertCount);
-        buffer.put(signBit ? (byte)0x80 : (byte)0x00);
-        NBitIntegerEncoder.encode(buffer, 7, deltaBase);
-
-        // Encode the field lines into the ByteBuffer.
-        for (EncodableEntry entry : encodableEntries)
         {
-            entry.encode(buffer, base);
+            // TODO: We have already added to the dynamic table so we need to send the instructions to maintain correct state.
+            //  Can we prevent adding to the table until we know the buffer has enough space?
+            notifyInstructionHandler();
+            streamInfo.remove(sectionInfo);
+            sectionInfo.release();
+            throw new QpackException.StreamException(H3_GENERAL_PROTOCOL_ERROR, "buffer_space_exceeded", e);
+        }
+        catch (Throwable t)
+        {
+            // We are failing the whole Session so don't need to send instructions back.
+            throw new QpackException.SessionException(H3_GENERAL_PROTOCOL_ERROR, "compression_error", t);
         }
-
-        notifyInstructionHandler();
     }
 
     /**
@@ -195,11 +217,22 @@ public class QpackEncoder implements Dumpable
      */
     public void parseInstructionBuffer(ByteBuffer buffer) throws QpackException
     {
-        while (BufferUtil.hasContent(buffer))
+        try
         {
-            _parser.parse(buffer);
+            while (BufferUtil.hasContent(buffer))
+            {
+                _parser.parse(buffer);
+            }
+            notifyInstructionHandler();
+        }
+        catch (QpackException.SessionException e)
+        {
+            throw e;
+        }
+        catch (Throwable t)
+        {
+            throw new QpackException.SessionException(QPACK_DECODER_STREAM_ERROR, t.getMessage(), t);
         }
-        notifyInstructionHandler();
     }
 
     /**
@@ -211,15 +244,15 @@ public class QpackEncoder implements Dumpable
     public boolean insert(HttpField field)
     {
         DynamicTable dynamicTable = _context.getDynamicTable();
-
         if (field.getValue() == null)
             field = new HttpField(field.getHeader(), field.getName(), "");
 
+        // If we should not index this entry or there is no room to insert it, then just return false.
         boolean canCreateEntry = shouldIndex(field) && dynamicTable.canInsert(field);
         if (!canCreateEntry)
             return false;
 
-        // We can always reference on insertion as it will always arrive before any eviction.
+        // Can we insert by duplicating an existing entry?
         Entry entry = _context.get(field);
         if (entry != null)
         {
@@ -230,6 +263,7 @@ public class QpackEncoder implements Dumpable
             return true;
         }
 
+        // Can we insert by referencing a name?
         boolean huffman = shouldHuffmanEncode(field);
         Entry nameEntry = _context.get(field.getName());
         if (nameEntry != null)
@@ -241,9 +275,9 @@ public class QpackEncoder implements Dumpable
             return true;
         }
 
+        // Add the entry without referencing an existing entry.
         dynamicTable.add(new Entry(field));
         _instructions.add(new LiteralNameEntryInstruction(field, huffman));
-
         notifyInstructionHandler();
         return true;
     }
@@ -265,8 +299,6 @@ public class QpackEncoder implements Dumpable
         if (field.getValue() == null)
             field = new HttpField(field.getHeader(), field.getName(), "");
 
-        // TODO: The field.getHeader() could be null.
-
         if (field instanceof PreEncodedHttpField)
             return EncodableEntry.getPreEncodedEntry((PreEncodedHttpField)field);
 
@@ -431,7 +463,7 @@ public class QpackEncoder implements Dumpable
         _instructions.clear();
     }
 
-    public class EncoderAdapter implements EncoderInstructionParser.Handler
+    class InstructionHandler implements EncoderInstructionParser.Handler
     {
         @Override
         public void onSectionAcknowledgement(long streamId) throws QpackException

jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/internal/StreamInfo.java

@@ -41,6 +41,11 @@ public class StreamInfo implements Iterable<StreamInfo.SectionInfo>
         _sectionInfos.add(sectionInfo);
     }
 
+    public void remove(SectionInfo sectionInfo)
+    {
+        _sectionInfos.remove(sectionInfo);
+    }
+
     public SectionInfo getCurrentSectionInfo()
     {
         return _sectionInfos.peek();

jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/internal/parser/EncodedFieldSection.java

@@ -187,7 +187,7 @@ public class EncodedFieldSection
 
     public interface EncodedField
     {
-        HttpField decode(QpackContext context) throws QpackException;
+        HttpField decode(QpackContext context);
     }
 
     private static class LiteralField implements EncodedField
@@ -222,7 +222,7 @@ public class EncodedFieldSection
         }
 
         @Override
-        public HttpField decode(QpackContext context) throws QpackException
+        public HttpField decode(QpackContext context)
         {
             if (_dynamicTable)
                 return context.getDynamicTable().getAbsolute(_base - (_index + 1)).getHttpField();
@@ -241,7 +241,7 @@ public class EncodedFieldSection
         }
 
         @Override
-        public HttpField decode(QpackContext context) throws QpackException
+        public HttpField decode(QpackContext context)
         {
             return context.getDynamicTable().getAbsolute(_base + _index).getHttpField();
         }
@@ -264,7 +264,7 @@ public class EncodedFieldSection
         }
 
         @Override
-        public HttpField decode(QpackContext context) throws QpackException
+        public HttpField decode(QpackContext context)
         {
             HttpField field;
             if (_dynamicTable)
@@ -290,7 +290,7 @@ public class EncodedFieldSection
         }
 
         @Override
-        public HttpField decode(QpackContext context) throws QpackException
+        public HttpField decode(QpackContext context)
         {
             HttpField field = context.getDynamicTable().getAbsolute(_base + _nameIndex).getHttpField();
             return new HttpField(field.getHeader(), field.getName(), _value);