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.

Sign up for GitHub

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

Jump to bottom

Avoid exposing HPACK classes #6591

Closed
Scottmitch opened this issue Mar 31, 2017 · 8 comments
Closed

Avoid exposing HPACK classes #6591

Scottmitch opened this issue Mar 31, 2017 · 8 comments
Assignees
@Scottmitch
Labels
improvement

Comments

@Scottmitch
Copy link
Member

HPACK is an internal detail of HTTP/2 and if possible it would be nice to avoid exposing this in the public interface of the codec-http2. Recently PR #6589 exposed HpackDecoder for extensibility reasons. In this particular case the extensibility point seemed to be focused on creating a different type of Http2Headers object than DefaultHttp2Headers. I would like to propose an alternative approach to exposing HpackDecoder and instead make DefaultHttp2HeadersDecoder's creation of the Http2Header object overridable. In this particular case GrpcHttp2HeadersDecoder is duplicating logic which also exists in DefaultHttp2HeadersDecoder and IMO would be nice to remove this duplication and consolidate the logic in DefaultHttp2HeadersDecoder.

Related:
#6579
#6589
@Scottmitch
Copy link
Member Author

@carl-mastrangelo @nmittler - I would like to propose reverting #6589 and instead submit a new PR which makes the necessary changes to DefaultHttp2HeadersDecoder. WDYT?

@Scottmitch Scottmitch mentioned this issue Mar 31, 2017
Merged
@carl-mastrangelo
Copy link
Member

@Scottmitch I think the reason for that file was to make unsafe assumptions gRPC can safely make.

@Scottmitch
Copy link
Member Author

@carl-mastrangelo - Just to be sure I understand you...

I think the reason for that file

GrpcHttp2HeadersDecoder?

Can you clarify your thoughts on my suggestion above? Extending DefaultHttp2HeadersDecoder seems like a cleaner approach and will reduce duplication. Do you agree, and do you have concerns (e.g. performance, functionality missing, etc...)?

@carl-mastrangelo
Copy link
Member

The decodeHeaders forces construction of DefaultHttp2Headers, which is what we want to avoid. If there was an overload for the construction of the Http2Headers object that is populated, that would work too.

@ejona86
Copy link
Member

ejona86 commented Mar 31, 2017

@Scottmitch, your suggestion sounds fine. The main objective was just to override DefaultHttp2Headers. That is easy to accomplish in other ways (factory, or overriding a construction method). From what I can tell, that would allow avoiding HpackDecoder without any regression.

@Scottmitch
Copy link
Member Author

If there was an overload for the construction of the Http2Headers object that is populated

Yes this is my suggestion.

Thanks for the feedback @carl-mastrangelo @ejona86 ... it sounds like we are on the same page. Let me move ahead with this approach. @carl-mastrangelo - I will ping you on the PR so you can make sure you have the necessary extension points in gRPC.

@Scottmitch Scottmitch self-assigned this Apr 1, 2017
Scottmitch added a commit to Scottmitch/netty that referenced this issue Apr 1, 2017
@Scottmitch
HTTP/2 Make DefaultHttp2HeadersDecoder's Http2Headers object creation…
01dca55 
… extensible

Motivation:
It is generally useful to override DefaultHttp2HeadersDecoder's creation of a new Http2Headers object so more optimized versions can be substituted if the use case allows for it.

Modifications:
- DefaultHttp2HeadersDecoder should support an overridable method to generate the new Http2Headers object for each decode operation

Result:
DefaultHttp2HeadersDecoder is more extensible.
Fixes netty#6591.
@Scottmitch Scottmitch mentioned this issue Apr 1, 2017
Closed
@carl-mastrangelo
Copy link
Member

We are keen to start using this change; @normanmaurer is there an ETA for 4.1.10 ?

@Scottmitch
Copy link
Member Author

should be in the next couple weeks.

@Scottmitch Scottmitch mentioned this issue Apr 4, 2017
Closed
liuzhengyang pushed a commit to liuzhengyang/netty that referenced this issue Sep 10, 2017
@Scottmitch
HTTP/2 Make DefaultHttp2HeadersDecoder's Http2Headers object creation…
af58052 
… extensible

Motivation:
It is generally useful to override DefaultHttp2HeadersDecoder's creation of a new Http2Headers object so more optimized versions can be substituted if the use case allows for it.

Modifications:
- DefaultHttp2HeadersDecoder should support an overridable method to generate the new Http2Headers object for each decode operation

Result:
DefaultHttp2HeadersDecoder is more extensible.
Fixes netty#6591.
kiril-me pushed a commit to kiril-me/netty that referenced this issue Feb 28, 2018
@Scottmitch @kiril-me
HTTP/2 Make DefaultHttp2HeadersDecoder's Http2Headers object creation…
765c59e 
… extensible

Motivation:
It is generally useful to override DefaultHttp2HeadersDecoder's creation of a new Http2Headers object so more optimized versions can be substituted if the use case allows for it.

Modifications:
- DefaultHttp2HeadersDecoder should support an overridable method to generate the new Http2Headers object for each decode operation

Result:
DefaultHttp2HeadersDecoder is more extensible.
Fixes netty#6591.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Scottmitch Scottmitch

Labels
improvement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@ejona86 @Scottmitch @carl-mastrangelo