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

Include SETTINGS_MAX_HEADER_LIST_SIZE in initial SETTINGS. #2350

Closed
buchgr opened this issue Oct 17, 2016 · 4 comments
Closed

Include SETTINGS_MAX_HEADER_LIST_SIZE in initial SETTINGS. #2350

buchgr opened this issue Oct 17, 2016 · 4 comments
Assignees
Labels
Milestone

Comments

@buchgr
Copy link
Collaborator

buchgr commented Oct 17, 2016

We currently don't send SETTINGS_MAX_HEADER_LIST_SIZE as part of the initial SETTINGS frame, although we enforce it on the decoder. This is valid as of the H2 spec

For any given request, a lower limit than what is advertised MAY be enforced. The initial value of this setting is unlimited.

However, we would argue that we should still include it in the initial SETTINGS frame, cause why not? If both endpoints are gRPC Java, it would also give us better error messages, as then the local encoder would enforce the remote decode limit [1]. However, we can't currently do that cause it seems like a bug in Netty that the encoder throws a connection error and not a stream error [1], bringing down the whole connection, while only the stream should fail. I ll open a issue/PR in Netty.

[1] https://github.com/netty/netty/blob/4.1/codec-http2/src/main/java/io/netty/handler/codec/http2/internal/hpack/Encoder.java#L126

@buchgr buchgr added the netty label Oct 17, 2016
@buchgr
Copy link
Collaborator Author

buchgr commented Oct 17, 2016

Opened Netty PR netty/netty#5919

@ejona86
Copy link
Member

ejona86 commented Oct 25, 2016

Does anything need to be done here? Will the next update of Netty "just work"?

@buchgr
Copy link
Collaborator Author

buchgr commented Oct 25, 2016

@ejona86 We need to also include the max header list value in the initial settings. So add one line in client handler and one line in server handler. [1]

initialSettings.maxHeaderListSize(maxHeaderListSize);

[1] https://github.com/grpc/grpc-java/blob/master/netty/src/main/java/io/grpc/netty/NettyClientHandler.java#L156

@lukaszx0
Copy link
Collaborator

lukaszx0 commented Mar 17, 2017

For posterity and people that are using versions < 1.2.x.

This fixes following stream error which gets thrown when header size is exceeded:

WARNING: Stream Error
io.netty.handler.codec.http2.Http2Exception$HeaderListSizeException: Header size exceeded max allowed size (8192)
at io.netty.handler.codec.http2.Http2Exception.headerListSizeError(Http2Exception.java:169)
at io.netty.handler.codec.http2.Http2CodecUtil.headerListSizeExceeded(Http2CodecUtil.java:252)
at io.netty.handler.codec.http2.internal.hpack.Encoder.encodeHeadersEnforceMaxHeaderListSize(Encoder.java:128)
at io.netty.handler.codec.http2.internal.hpack.Encoder.encodeHeaders(Encoder.java:112)
at io.netty.handler.codec.http2.DefaultHttp2HeadersEncoder.encodeHeaders(DefaultHttp2HeadersEncoder.java:69)
at io.netty.handler.codec.http2.DefaultHttp2FrameWriter.writeHeadersInternal(DefaultHttp2FrameWriter.java:435)
  ...

@ejona86 ejona86 modified the milestones: 1.2, Next Apr 5, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Sep 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants