From 6a825df16e8dd7ee2dc8b67491bfc59bc5197d38 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 14 Sep 2021 16:09:53 +1000 Subject: [PATCH] Improve javadoc and exception handling for http3-qpack Signed-off-by: Lachlan Roberts --- .../jetty/http3/qpack/Instruction.java | 6 + .../jetty/http3/qpack/QpackDecoder.java | 43 +++---- .../jetty/http3/qpack/QpackEncoder.java | 112 +++++++++++------- .../http3/qpack/internal/StreamInfo.java | 5 + .../internal/parser/EncodedFieldSection.java | 10 +- 5 files changed, 110 insertions(+), 66 deletions(-) diff --git a/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/Instruction.java b/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/Instruction.java index bb2d1807c8b..04d0ff46da7 100644 --- a/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/Instruction.java +++ b/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); + /** + *

A handler for instructions issued by an {@link QpackEncoder} or {@link QpackDecoder}.

+ *

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.

+ */ interface Handler { void onInstructions(List instructions); diff --git a/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/QpackDecoder.java b/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/QpackDecoder.java index 710d5764afa..b9535fda05e 100644 --- a/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/QpackDecoder.java +++ b/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(); - notifyMetaDataHandler(); - throw t; + throw new QpackException.SessionException(QPACK_ENCODER_STREAM_ERROR, t.getMessage(), 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) 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 97c204a29d2..9ac136fcca3 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 @@ -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 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; - - // This will also extract pseudo headers from the metadata. - Http3Fields httpFields = new Http3Fields(metadata); - for (HttpField field : httpFields) + try { - EncodableEntry entry = encode(streamInfo, field); - encodableEntries.add(entry); + int requiredInsertCount = 0; + for (HttpField field : new Http3Fields(metadata)) + { + EncodableEntry entry = encode(streamInfo, field); + encodableEntries.add(entry); - // Update the required InsertCount. - int entryRequiredInsertCount = entry.getRequiredInsertCount(); - if (entryRequiredInsertCount > requiredInsertCount) - requiredInsertCount = entryRequiredInsertCount; + // Update the required InsertCount. + int entryRequiredInsertCount = entry.getRequiredInsertCount(); + if (entryRequiredInsertCount > requiredInsertCount) + 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(); } - - 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) + catch (BufferOverflowException e) { - 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 diff --git a/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/internal/StreamInfo.java b/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/internal/StreamInfo.java index c5a0ddd9fd6..11d3a7545cf 100644 --- a/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/internal/StreamInfo.java +++ b/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/internal/StreamInfo.java @@ -41,6 +41,11 @@ public class StreamInfo implements Iterable _sectionInfos.add(sectionInfo); } + public void remove(SectionInfo sectionInfo) + { + _sectionInfos.remove(sectionInfo); + } + public SectionInfo getCurrentSectionInfo() { return _sectionInfos.peek(); diff --git a/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/internal/parser/EncodedFieldSection.java b/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/internal/parser/EncodedFieldSection.java index 0bcbf0190ab..bdca9aab539 100644 --- a/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/internal/parser/EncodedFieldSection.java +++ b/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);