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

DefaultHttp2ConnectionEncoder writeHeaders method always send an header frame with a priority #9842

Closed
vietj opened this issue Dec 4, 2019 · 6 comments · Fixed by #9852
Closed
Milestone

Comments

@vietj
Copy link
Contributor

vietj commented Dec 4, 2019

Expected behavior

The DefaultHttp2ConnectionEncoder sends an header frame without priority when the writeHeaders(ChannelHandlerContext ctx, int streamId, Http2Headers headers, int padding, boolean endStream, ChannelPromise promise) method is used

Actual behavior

The current implementation delegates to writeHeaders(ChannelHandlerContext ctx, int streamId, Http2Headers headers, int streamDependency, short weight, boolean exclusive, int padding, boolean endStream, ChannelPromise promise) that will send an header frame with the priority flag set and the default priority values.

Steps to reproduce

Use DefaultHttp2ConnectionEncoder#writeHeaders(ChannelHandlerContext ctx, int streamId, Http2Headers headers, int padding, boolean endStream, ChannelPromise promise) with an Http2FrameWriter, the frame writer will have the writeHeaders(ChannelHandlerContext ctx, int streamId, Http2Headers headers, int streamDependency, short weight, boolean exclusive, int padding, boolean endStream, ChannelPromise promise) called.

On the wire, this will encode a different headers frame with the priority flag set instead of being unset.

Minimal yet complete reproducer code (or URL to code)

I believe this in DefaultHttp2ConnectionEncoderTest should do the job

    @Test
    public void headersWithNoPriority() {
        writeAllFlowControlledFrames();
        final int streamId = 6;
        ChannelPromise promise = newPromise();
        encoder.writeHeaders(ctx, streamId, EmptyHttp2Headers.INSTANCE, 0, false, promise);
        verify(writer, times(1)).writeHeaders(eq(ctx), eq(streamId), eq(EmptyHttp2Headers.INSTANCE), eq(0), eq(false), eq(promise));
    }

This fails with

Argument(s) are different! Wanted:
writer.writeHeaders(
    ctx,
    6,
    EmptyHttp2Headers[],
    0,
    false,
    DefaultChannelPromise@2374d36a(success)
);
-> at io.netty.handler.codec.http2.DefaultHttp2ConnectionEncoderTest.headersWithNoPriority(DefaultHttp2ConnectionEncoderTest.java:403)
Actual invocation has different arguments:
writer.writeHeaders(
    ctx,
    6,
    EmptyHttp2Headers[],
    0,
    (short) 16,
    false,
    0,
    false,
    DefaultChannelPromise@2374d36a(success)
);
-> at io.netty.handler.codec.http2.DefaultHttp2ConnectionEncoder.writeHeaders(DefaultHttp2ConnectionEncoder.java:208)

Netty version

All of 4.1 branch

JVM version (e.g. java -version)

Does not matter

OS version (e.g. uname -a)

Does not matter

@vietj
Copy link
Contributor Author

vietj commented Dec 4, 2019

@normanmaurer
Copy link
Member

@vietj sounds about right... interested in doing a PR ?

@vietj
Copy link
Contributor Author

vietj commented Dec 4, 2019

yes I will try soon.

@normanmaurer
Copy link
Member

/cc @bryce-anderson @ejona86

@normanmaurer
Copy link
Member

@vietj don't worry... working on it

@normanmaurer
Copy link
Member

@vietj PTAL #9852

normanmaurer added a commit that referenced this issue Dec 6, 2019
…er frame with a priority

Motivation:

The current implementation delegates to writeHeaders(ChannelHandlerContext ctx, int streamId, Http2Headers headers, int streamDependency, short weight, boolean exclusive, int padding, boolean endStream, ChannelPromise promise) that will send an header frame with the priority flag set and the default priority values even if the user didnt want too.

Modifications:

- Change DefaultHttp2ConnectionEncoder to call the correct Http2FrameWriter method depending on if the user wants to use priorities or not
- Adjust tests

Result:

Fixes #9842
normanmaurer added a commit that referenced this issue Dec 8, 2019
… frame with a priority (#9852)

Motivation:

The current implementation delegates to writeHeaders(ChannelHandlerContext ctx, int streamId, Http2Headers headers, int streamDependency, short weight, boolean exclusive, int padding, boolean endStream, ChannelPromise promise) that will send an header frame with the priority flag set and the default priority values even if the user didnt want too.

Modifications:

- Change DefaultHttp2ConnectionEncoder to call the correct Http2FrameWriter method depending on if the user wants to use priorities or not
- Adjust tests

Result:

Fixes #9842
@normanmaurer normanmaurer added this to the 4.1.44.Final milestone Dec 8, 2019
normanmaurer added a commit that referenced this issue Dec 8, 2019
… frame with a priority (#9852)

Motivation:

The current implementation delegates to writeHeaders(ChannelHandlerContext ctx, int streamId, Http2Headers headers, int streamDependency, short weight, boolean exclusive, int padding, boolean endStream, ChannelPromise promise) that will send an header frame with the priority flag set and the default priority values even if the user didnt want too.

Modifications:

- Change DefaultHttp2ConnectionEncoder to call the correct Http2FrameWriter method depending on if the user wants to use priorities or not
- Adjust tests

Result:

Fixes #9842
ihanyong pushed a commit to ihanyong/netty that referenced this issue Jul 31, 2020
… frame with a priority (netty#9852)

Motivation:

The current implementation delegates to writeHeaders(ChannelHandlerContext ctx, int streamId, Http2Headers headers, int streamDependency, short weight, boolean exclusive, int padding, boolean endStream, ChannelPromise promise) that will send an header frame with the priority flag set and the default priority values even if the user didnt want too.

Modifications:

- Change DefaultHttp2ConnectionEncoder to call the correct Http2FrameWriter method depending on if the user wants to use priorities or not
- Adjust tests

Result:

Fixes netty#9842
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 a pull request may close this issue.

2 participants