From c271b70d298ff5e9ad78b6bb96b24b2d23473f9e Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Wed, 3 Mar 2021 13:49:34 +1100 Subject: [PATCH] Rewrite QpackEncoder.encode for HttpField, implement Duplicates. Signed-off-by: Lachlan Roberts --- .../jetty/http3/qpack/QpackEncoder.java | 116 +++++++++++++----- .../parser/DecoderInstructionParser.java | 35 ++++++ .../parser/EncoderInstructionParser.java | 1 - .../jetty/http3/qpack/table/Entry.java | 7 +- .../qpack/DecoderInstructionParserTest.java | 3 + .../jetty/http3/qpack/EncodeDecodeTest.java | 5 +- .../http3/qpack/NBitIntegerParserTest.java | 1 - 7 files changed, 135 insertions(+), 33 deletions(-) diff --git a/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/QpackEncoder.java b/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/QpackEncoder.java index 67d9782e3ac..05b5c412692 100644 --- a/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/QpackEncoder.java +++ b/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/QpackEncoder.java @@ -28,6 +28,7 @@ import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.HttpMethod; import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.PreEncodedHttpField; +import org.eclipse.jetty.http3.qpack.generator.DuplicateInstruction; import org.eclipse.jetty.http3.qpack.generator.IndexedNameEntryInstruction; import org.eclipse.jetty.http3.qpack.generator.Instruction; import org.eclipse.jetty.http3.qpack.generator.LiteralNameEntryInstruction; @@ -105,6 +106,7 @@ public class QpackEncoder private final int _maxBlockedStreams; private final Map _blockedStreams = new HashMap<>(); private boolean _validateEncoding = true; + private int _knownInsertCount; public QpackEncoder(Handler handler, int maxBlockedStreams) { @@ -117,6 +119,7 @@ public class QpackEncoder _bufferPool = bufferPool; _context = new QpackContext(); _maxBlockedStreams = maxBlockedStreams; + _knownInsertCount = 0; } private boolean acquireBlockedStream(int streamId) @@ -151,6 +154,21 @@ public class QpackEncoder _handler.onInstruction(new SetCapacityInstruction(capacity)); } + public void insertCountIncrement(int increment) + { + _knownInsertCount += increment; + } + + public void sectionAcknowledgement(int streamId) + { + // TODO: Implement. + } + + public void streamCancellation(int streamId) + { + // TODO: Implement. + } + public QpackContext getQpackContext() { return _context; @@ -166,6 +184,31 @@ public class QpackEncoder _validateEncoding = validateEncoding; } + public boolean referenceEntry(Entry entry, int streamId) + { + if (entry == null) + return false; + + if (entry.isStatic()) + return true; + + boolean inEvictionZone = !_context.getDynamicTable().canReference(entry); + if (inEvictionZone) + return false; + + if (_knownInsertCount >= entry.getIndex() + 1) + { + entry.reference(); + return true; + } + + // We might be able to risk blocking the decoder stream and reference this immediately. + boolean riskBlockedStream = acquireBlockedStream(streamId); + if (riskBlockedStream) + entry.reference(); + return riskBlockedStream; + } + public static boolean shouldIndex(HttpField httpField) { return !DO_NOT_INDEX.contains(httpField.getHeader()); @@ -241,45 +284,62 @@ public class QpackEncoder // TODO: // 1. The field.getHeader() could be null. // 3. Handle pre-encoded HttpFields. - // 4. Someone still needs to generate the HTTP/3 pseudo headers. (this should be independent of HTTP/3 though?) + + boolean canCreateEntry = shouldIndex(field) && (Entry.getSize(field) <= dynamicTable.getSpace()); Entry entry = _context.get(field); - if (entry != null && _context.canReference(entry)) + if (entry != null && referenceEntry(entry, streamId)) { - // TODO: we may want to duplicate the entry if it is in the eviction zone? - // then we would also need to reference this entry, is that okay? - entry.reference(); return new EncodableEntry(entry); } - - Entry nameEntry = _context.get(field.getName()); - boolean canReferenceName = nameEntry != null && _context.canReference(nameEntry); - - Entry newEntry = new Entry(field); - if (shouldIndex(field) && (newEntry.getSize() <= dynamicTable.getSpace())) + else { - dynamicTable.add(newEntry); - - boolean huffman = shouldHuffmanEncode(field); - if (canReferenceName) + // Should we duplicate this entry. + if (entry != null && dynamicTable.canReference(entry) && canCreateEntry) { - boolean isDynamic = !nameEntry.isStatic(); - int nameIndex = _context.index(nameEntry); - _handler.onInstruction(new IndexedNameEntryInstruction(isDynamic, nameIndex, huffman, field.getValue())); - } - else - { - _handler.onInstruction(new LiteralNameEntryInstruction(huffman, field.getName(), huffman, field.getValue())); - } + Entry newEntry = new Entry(field); + dynamicTable.add(newEntry); + _handler.onInstruction(new DuplicateInstruction(entry.getIndex())); - // We might be able to risk blocking the decoder stream and reference this immediately. - if (acquireBlockedStream(streamId)) - return new EncodableEntry(newEntry); + // Should we reference this entry and risk blocking. + if (referenceEntry(newEntry, streamId)) + return new EncodableEntry(newEntry); + } } - if (canReferenceName) + boolean huffman = shouldHuffmanEncode(field); + Entry nameEntry = _context.get(field.getName()); + if (nameEntry != null && referenceEntry(nameEntry, streamId)) + { + // Should we copy this entry + if (canCreateEntry) + { + Entry newEntry = new Entry(field); + dynamicTable.add(newEntry); + _handler.onInstruction(new IndexedNameEntryInstruction(!nameEntry.isStatic(), nameEntry.getIndex(), huffman, field.getValue())); + + // Should we reference this entry and risk blocking. + if (referenceEntry(newEntry, streamId)) + return new EncodableEntry(newEntry); + } + return new EncodableEntry(nameEntry, field); - return new EncodableEntry(field); + } + else + { + if (canCreateEntry) + { + Entry newEntry = new Entry(field); + dynamicTable.add(newEntry); + _handler.onInstruction(new LiteralNameEntryInstruction(huffman, field.getName(), huffman, field.getValue())); + + // Should we reference this entry and risk blocking. + if (referenceEntry(newEntry, streamId)) + return new EncodableEntry(newEntry); + } + + return new EncodableEntry(field); + } } public static int encodeInsertCount(int reqInsertCount, int maxTableCapacity) diff --git a/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/parser/DecoderInstructionParser.java b/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/parser/DecoderInstructionParser.java index 82730e44efc..ec132957459 100644 --- a/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/parser/DecoderInstructionParser.java +++ b/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/parser/DecoderInstructionParser.java @@ -15,6 +15,8 @@ package org.eclipse.jetty.http3.qpack.parser; import java.nio.ByteBuffer; +import org.eclipse.jetty.http3.qpack.QpackEncoder; + /** * Receives instructions coming from the remote Encoder as a sequence of unframed instructions. */ @@ -45,6 +47,39 @@ public class DecoderInstructionParser void onInsertCountIncrement(int increment); } + public static class EncoderHandler implements Handler + { + private final QpackEncoder _encoder; + + public EncoderHandler(QpackEncoder encoder) + { + _encoder = encoder; + } + + @Override + public void onSectionAcknowledgement(int streamId) + { + _encoder.sectionAcknowledgement(streamId); + } + + @Override + public void onStreamCancellation(int streamId) + { + _encoder.streamCancellation(streamId); + } + + @Override + public void onInsertCountIncrement(int increment) + { + _encoder.insertCountIncrement(increment); + } + } + + public DecoderInstructionParser(QpackEncoder encoder) + { + this(new EncoderHandler(encoder)); + } + public DecoderInstructionParser(Handler handler) { _handler = handler; diff --git a/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/parser/EncoderInstructionParser.java b/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/parser/EncoderInstructionParser.java index 33a0b639e44..2a40ea8c7be 100644 --- a/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/parser/EncoderInstructionParser.java +++ b/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/parser/EncoderInstructionParser.java @@ -100,7 +100,6 @@ public class EncoderInstructionParser this(new DecoderHandler(decoder)); } - public EncoderInstructionParser(Handler handler) { _handler = handler; diff --git a/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/table/Entry.java b/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/table/Entry.java index ca2e5c8d86b..9c58a0308c1 100644 --- a/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/table/Entry.java +++ b/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/table/Entry.java @@ -42,7 +42,7 @@ public class Entry public int getSize() { - return 32 + StringUtil.getLength(_field.getName()) + StringUtil.getLength(_field.getValue()); + return getSize(_field); } public void setIndex(int index) @@ -85,4 +85,9 @@ public class Entry { return String.format("{%s,%d,%s,%x}", isStatic() ? "S" : "D", _absoluteIndex, _field, hashCode()); } + + public static int getSize(HttpField field) + { + return 32 + StringUtil.getLength(field.getName()) + StringUtil.getLength(field.getValue()); + } } diff --git a/jetty-http3/http3-qpack/src/test/java/org/eclipse/jetty/http3/qpack/DecoderInstructionParserTest.java b/jetty-http3/http3-qpack/src/test/java/org/eclipse/jetty/http3/qpack/DecoderInstructionParserTest.java index ca19e5a8afb..4fa95f2399e 100644 --- a/jetty-http3/http3-qpack/src/test/java/org/eclipse/jetty/http3/qpack/DecoderInstructionParserTest.java +++ b/jetty-http3/http3-qpack/src/test/java/org/eclipse/jetty/http3/qpack/DecoderInstructionParserTest.java @@ -20,6 +20,7 @@ import java.util.Queue; import org.eclipse.jetty.http3.qpack.parser.DecoderInstructionParser; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.TypeUtil; +import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import static org.hamcrest.MatcherAssert.assertThat; @@ -114,6 +115,7 @@ public class DecoderInstructionParserTest } } + @Disabled @Test public void testStreamCancellation() throws Exception { @@ -121,6 +123,7 @@ public class DecoderInstructionParserTest throw new RuntimeException("TODO: testStreamCancellation"); } + @Disabled @Test public void testInsertCountIncrement() throws Exception { diff --git a/jetty-http3/http3-qpack/src/test/java/org/eclipse/jetty/http3/qpack/EncodeDecodeTest.java b/jetty-http3/http3-qpack/src/test/java/org/eclipse/jetty/http3/qpack/EncodeDecodeTest.java index 56e259dd307..fc9847e777b 100644 --- a/jetty-http3/http3-qpack/src/test/java/org/eclipse/jetty/http3/qpack/EncodeDecodeTest.java +++ b/jetty-http3/http3-qpack/src/test/java/org/eclipse/jetty/http3/qpack/EncodeDecodeTest.java @@ -47,8 +47,8 @@ public class EncodeDecodeTest private DecoderInstructionParser _decoderInstructionParser; private EncoderInstructionParser _encoderInstructionParser; - private final int MAX_BLOCKED_STREAMS = 5; - private final int MAX_HEADER_SIZE = 1024; + private static final int MAX_BLOCKED_STREAMS = 5; + private static final int MAX_HEADER_SIZE = 1024; @BeforeEach public void before() @@ -58,6 +58,7 @@ public class EncodeDecodeTest _encoder = new QpackEncoder(_encoderHandler, MAX_BLOCKED_STREAMS); _decoder = new QpackDecoder(_decoderHandler, MAX_HEADER_SIZE); _encoderInstructionParser = new EncoderInstructionParser(_decoder); + _decoderInstructionParser = new DecoderInstructionParser(_encoder); } @Test diff --git a/jetty-http3/http3-qpack/src/test/java/org/eclipse/jetty/http3/qpack/NBitIntegerParserTest.java b/jetty-http3/http3-qpack/src/test/java/org/eclipse/jetty/http3/qpack/NBitIntegerParserTest.java index e615e994342..744c382a3d9 100644 --- a/jetty-http3/http3-qpack/src/test/java/org/eclipse/jetty/http3/qpack/NBitIntegerParserTest.java +++ b/jetty-http3/http3-qpack/src/test/java/org/eclipse/jetty/http3/qpack/NBitIntegerParserTest.java @@ -41,7 +41,6 @@ public class NBitIntegerParserTest int value = parser.decode(buffer1); assertThat(value, is(-1)); - parser.setPrefix(7); value = parser.decode(buffer2); assertThat(value, is(1337)); }