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

DefaultHttp2Connection.DefaultEndpoint#lastStreamCreated return 0 when the actual stream id is Integer.MAX_VALUE #13805

Closed
DreamLettuce opened this issue Jan 24, 2024 · 4 comments · Fixed by #13807
Assignees
Milestone

Comments

@DreamLettuce
Copy link

DreamLettuce commented Jan 24, 2024

Expected behavior

when stream id is Integer.MAX_VALUE, DefaultHttp2Connection.DefaultEndpoint#lastStreamCreated is expected to return Integer.MAX_VALUE
image

Actual behavior

  1. DefaultHttp2Connection.DefaultEndpoint#lastStreamCreated return 0
  2. io.netty.handler.codec.http2.StreamBufferingEncoder#isExistingStream return false
  3. StreamBufferingEncoder write data frame failed
image
  1. the further problem is that grpc-java does not handle this write data failure in NettyClientStream
    .this result to the grpc client thread hangs forever is we use grpc blocking stub

Steps to reproduce

add the following test case to io.netty.handler.codec.http2.StreamBufferingEncoderTest

    @Test
    public void testExhaustedStreamId() throws Http2Exception {
        int nextStreamId = Integer.MAX_VALUE - 2;
        testStreamId(nextStreamId);
        nextStreamId = connection.local().incrementAndGetNextStreamId();
        testStreamId(nextStreamId);
    }

    private void testStreamId(int nextStreamId) throws Http2Exception {
        connection.local().createStream(nextStreamId, false);
        ByteBuf data = data();
        ChannelFuture channelFuture = encoder.writeData(ctx, nextStreamId, data, 0, false, newPromise());
        if (channelFuture.cause() != null) {
            channelFuture.cause().printStackTrace();
        }
        assertTrue(channelFuture.cause() == null);
    }
image image

Minimal yet complete reproducer code (or URL to code)

Netty version

any support http2

JVM version (e.g. java -version)

1.8

OS version (e.g. uname -a)

@PaulTan94
Copy link

a very critical issue, any suggestion pls?

@normanmaurer
Copy link
Member

PTAL #13807

@normanmaurer normanmaurer added this to the 4.1.107.Final milestone Jan 25, 2024
@normanmaurer normanmaurer self-assigned this Jan 25, 2024
normanmaurer added a commit that referenced this issue Jan 25, 2024
…m ids were used

Motivation:

We did not correctly calculate the value returned by DefaultHttp2Connection.DefaultEndpoint.lastStreamCreated() and so returned 0 when all stream ids were used. This did lead to also return an result when checking if a stream might have been existed. As some encoders / decoders also use these methods to determine if a stream existed or not it also resulted in http2 errors.

Modifications:

- Correctly implement lastStreamCreated()
- Add test cases.

Result:

Fixes #13805
@normanmaurer
Copy link
Member

@DreamLettuce you should also report the other issue in GRPC /cc @ejona86

@DreamLettuce
Copy link
Author

PTAL #13807

LGTM 👍

normanmaurer added a commit that referenced this issue Jan 25, 2024
#13807)

…m ids were used

Motivation:

We did not correctly calculate the value returned by
DefaultHttp2Connection.DefaultEndpoint.lastStreamCreated() and so
returned 0 when all stream ids were used. This did lead to also return
an result when checking if a stream might have been existed. As some
encoders / decoders also use these methods to determine if a stream
existed or not it also resulted in http2 errors.

Modifications:

- Correctly implement lastStreamCreated()
- Add test cases.

Result:

Fixes #13805
normanmaurer added a commit that referenced this issue Jan 25, 2024
#13807)

…m ids were used

Motivation:

We did not correctly calculate the value returned by
DefaultHttp2Connection.DefaultEndpoint.lastStreamCreated() and so
returned 0 when all stream ids were used. This did lead to also return
an result when checking if a stream might have been existed. As some
encoders / decoders also use these methods to determine if a stream
existed or not it also resulted in http2 errors.

Modifications:

- Correctly implement lastStreamCreated()
- Add test cases.

Result:

Fixes #13805
franz1981 pushed a commit to franz1981/netty that referenced this issue Feb 9, 2024
netty#13807)

…m ids were used

Motivation:

We did not correctly calculate the value returned by
DefaultHttp2Connection.DefaultEndpoint.lastStreamCreated() and so
returned 0 when all stream ids were used. This did lead to also return
an result when checking if a stream might have been existed. As some
encoders / decoders also use these methods to determine if a stream
existed or not it also resulted in http2 errors.

Modifications:

- Correctly implement lastStreamCreated()
- Add test cases.

Result:

Fixes netty#13805
sergiitk added a commit to grpc/grpc-java that referenced this issue Apr 16, 2024
Handles Netty write frame failures caused by issues in the Netty
itself.

Normally we don't need to do anything on frame write failures because
the cause of a failed future would be an IO error that resulted in
the stream closure.  Prior to this PR we treated these issues as a
noop, except the initial headers write on the client side.

However, a case like netty/netty#13805 (a bug in generating next
stream id) resulted in an unclosed stream on our side. This PR adds
write frame future failure handlers that ensures the stream is
cancelled, and the cause is propagated via Status.

Fixes #10849
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.

3 participants