-
-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Static table only QPACK #3
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First quic review
src/main/java/io/netty/incubator/codec/http3/CharSequenceMap.java
Outdated
Show resolved
Hide resolved
src/main/java/io/netty/incubator/codec/http3/DefaultHttp3Headers.java
Outdated
Show resolved
Hide resolved
src/test/java/io/netty/incubator/codec/http3/QpackEncoderDecoderTest.java
Show resolved
Hide resolved
src/main/java/io/netty/incubator/codec/http3/DefaultHttp3Headers.java
Outdated
Show resolved
Hide resolved
public void validateName(CharSequence name) { | ||
if (name == null || name.length() == 0) { | ||
PlatformDependent.throwException( | ||
new Http3Exception(String.format("empty headers are not allowed [%s]", name))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's update the validateName
methods signature to include throws
declaration so we don't need to use this hack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Errors/exceptions handling definitely needs some love here. So far, it's mostly copy from HTTP/2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one turned out to be a pretty deep hole: it relies on an interface from codec
. I'm going to just leave it "as is" for now. We can change it later
@normanmaurer I pushed quite a significant change on how static table lookup works. See |
@kachayev looks great... let me know when you think its ready for a merge. We can do a followup later when we want to improve it / find bugs. |
@normanmaurer I will do another pass tomorrow, mainly to cleanup errors handling and validation. And will open issues for enhancements that I've mentioned in the very beginning. Also, I'm going to make a PR for |
Sounds good 👌 |
@kachayev wdyt about moving the Huffman stuff + CharSequenceMap into the codec package (just like the other compression stuff) so we could reuse it from h2 and h3. |
@normanmaurer Totally agree. I've mentioned this earlier
Decoder needs to be cleanup a bit as it throws HTTP/2 connection error, - this logic should stay in |
@normanmaurer I think it's good so far... Errors handling (e.g. validation exceptions you've mentioned) and general logic of |
|
||
private static final long DEFAULT_MAX_HEADER_LIST_SIZE = 0xffffffffL; | ||
|
||
private final QpackHuffmanDecoder huffmanDecoder = new QpackHuffmanDecoder(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify... This could be just the "standard" HuffmanDecoder that would be shared with http2 in the future, correct @kachayev ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly. The only thing we will need to change:
netty-incubator-codec-http3/src/main/java/io/netty/incubator/codec/http3/QpackHuffmanDecoder.java
Line 4649 in 8110ecc
private static final Http3Exception BAD_ENCODING = new Http3Exception("QPACK - Bad Encoding"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When moving to codec
package, this exception should be changed to something fairly generic. Meaning that the code should wrap decoding into try/catch to produce meaningful error information "locally"
Motivation:
QPACK is headers encoding algorithms essential for H3 protocol.
Modifications:
QpackEncoder
,QpackDecoder
andQpackStaticTable
, API is shown in the test case.Http3Headers
,DefaultHttp3Headers
are copy-pasted from HTTP/2 codec (corresponding classes). Not sure if we even need H3 headers to be defined separately or cleanup HTTP/2 implementation to have less dependencies on itself, e.g. HTTP/2-specific exceptions.HuffmanEncoder
andHuffmanDecoder
are copy-pastes as well. Those classes are package-private in HTTP/2 code and decoder uses HTTP/2-related exception. It would be nice to move both into utils or common instead of keep 2 separate copies. Same goes forCharSequenceMap
.Http3Exception
is some sort of a placeholder for future errors processing layer. Not sure if we want to follow HTTP/2 codec model though.QpackDecoder
is not implemented yet. Once again, it would be nice if we can cleanup HTTP/2 implementation to be able to re-use blocks of it (including headers sink and header validation logic).