Skip to content
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

Expose HTTP/2 HpackDecoder #6589

Merged
merged 2 commits into from
Mar 31, 2017
Merged

Expose HTTP/2 HpackDecoder #6589

merged 2 commits into from
Mar 31, 2017

Conversation

nmittler
Copy link
Member

@nmittler nmittler commented Mar 31, 2017

Motivation:

gRPC (and potentially other libraries) has an optimized header processor that requires direct access to the HpackDecoder.

Modifications:

Make the HpackDecoder and its constructors public.

Result:

Fixes #6579

Motivation:

gRPC (and potentially other libraries) has an optimized header processor that requires direct access to the HpackDecoder.

Modifications:

Make the HpackDecoder and its constructors public.

Result:

Fixes netty#6579
@nmittler nmittler added this to the 4.1.10.Final milestone Mar 31, 2017
@nmittler nmittler self-assigned this Mar 31, 2017
@@ -49,7 +49,7 @@
import static io.netty.util.internal.ObjectUtil.checkPositive;
import static io.netty.util.internal.ThrowableUtil.unknownStackTrace;

final class HpackDecoder {
public class HpackDecoder {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we make it public again can we at least make it final ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup - done. I wasn't sure at first whether gRPC needed to extend but it looks like it doesn't.

Copy link
Member

@Scottmitch Scottmitch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of exposing the details of HPACK would it be sufficient to make the headers created by DefaultHttp2HeadersDecoder extensible? DefaultHttp2HeadersDecoder is already public, and it looks like GrpcHttp2HeadersDecoder has some duplication anyways.

this(maxHeaderListSize, initialHuffmanDecodeCapacity, DEFAULT_HEADER_TABLE_SIZE);
}

/**
* Exposed Used for testing only! Default values used in the initial settings frame are overriden intentionally
* for testing but violate the RFC if used outside the scope of testing.
*/
HpackDecoder(long maxHeaderListSize, int initialHuffmanDecodeCapacity, int maxHeaderTableSize) {
public HpackDecoder(long maxHeaderListSize, int initialHuffmanDecodeCapacity, int maxHeaderTableSize) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

having this be public can be dangerous as indicated in the comments. Can this stay package private?

@Scottmitch
Copy link
Member

Lets track followup discussion in #6591

Scottmitch added a commit to Scottmitch/netty that referenced this pull request Apr 1, 2017
Scottmitch added a commit that referenced this pull request Apr 3, 2017
liuzhengyang pushed a commit to liuzhengyang/netty that referenced this pull request Sep 10, 2017
Motivation:

gRPC (and potentially other libraries) has an optimized header processor that requires direct access to the HpackDecoder.

Modifications:

Make the HpackDecoder and its constructors public.

Result:

Fixes netty#6579
liuzhengyang pushed a commit to liuzhengyang/netty that referenced this pull request Sep 10, 2017
kiril-me pushed a commit to kiril-me/netty that referenced this pull request Feb 28, 2018
Motivation:

gRPC (and potentially other libraries) has an optimized header processor that requires direct access to the HpackDecoder.

Modifications:

Make the HpackDecoder and its constructors public.

Result:

Fixes netty#6579
kiril-me pushed a commit to kiril-me/netty that referenced this pull request Feb 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants