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

Throwing protocol error when http2 max concurrent streams is exceeded instead of resStream #12065

Open
stefanklas opened this issue Feb 1, 2022 · 4 comments · Fixed by #13485
Milestone

Comments

@stefanklas
Copy link

stefanklas commented Feb 1, 2022

Expected behavior

I'm new to Http2, and was trying to implement some load shedding on our server by limiting the number of concurrent streams set. This is currently fixed at 100. When setting the maxConcurrentStream in the HttpToHttp2ConnectionHandler, I was expecting to get see the behaviour mentioned in the spec (https://httpwg.org/specs/rfc7540.html#rfc.section.8.1.4) and get a RST_STREAM frame back to the client.

Actual behavior

Instead i am getting an error, Stream ID does not exist. This results in a connection error, causing a GOAWAY with "PROTOCOL_ERROR" and closes the whole connection instead of just the stream? I have not understood if I am doing anything wrong here.

Screenshot 2022-02-01 at 15 50 15

Screenshot 2022-02-01 at 15 50 30

Screenshot 2022-02-01 at 16 32 55

Steps to reproduce

Minimal yet complete reproducer code (or URL to code)

Server Configuration

public class Http2OnlyHandler extends ApplicationProtocolNegotiationHandler {
 @Override
    protected void configurePipeline(ChannelHandlerContext ctx, String protocol) throws Exception {
        if (ApplicationProtocolNames.HTTP_2.equals(protocol)) {
            configureRedisPipeline(ctx);
            return;
        }

        if (ApplicationProtocolNames.HTTP_1_1.equals(protocol)) {
            configureFallback(ctx);
            return;
        }

        throw new IllegalStateException("Unsupported protocol: " + protocol);
    }


  private void configureRedisPipeline(ChannelHandlerContext ctx) {
  
    DefaultHttp2Connection connection = new DefaultHttp2Connection(true);
     InboundHttp2ToHttpAdapter listener = new InboundHttp2ToHttpAdapterBuilder(connection)
        .propagateSettings(true).validateHttpHeaders(false)
        .maxContentLength(maxContentLength).build();
  
    HttpToHttp2ConnectionHandler connectionHandler = new HttpToHttp2ConnectionHandlerBuilder()
        .frameListener(tester)
        .initialSettings(new Http2Settings().maxConcurrentStreams(100))
        .connection(connection).build();
  
    ctx.pipeline().addLast(connectionHandler);
  }
}


 bootstrap.group(bossGroup, workerGroup)
                .channel(channelClazz)
                .childHandler(new ChannelInitializer<SocketChannel>() {
                    @Override
                    protected void initChannel(SocketChannel ch) {
                        ch.pipeline().addLast(sslCtx.newHandler(ch.alloc()),
                            new Http2OnlyHandler(),
                            etc...)
                    }
                 })

Client:

channel.eventLoop().submit(() -> {
            int streamId = this.streamId.addAndGet(2);

            req.headers().setInt(HttpConversionUtil.ExtensionHeaderNames.STREAM_ID.text(), streamId);
            req.headers().set(HttpConversionUtil.ExtensionHeaderNames.SCHEME.text(), "https");
            req.headers().set(HttpHeaderNames.HOST, "server");
            req.headers().add(HttpHeaderNames.ACCEPT_ENCODING, HttpHeaderValues.GZIP);
            req.headers().add(HttpHeaderNames.ACCEPT_ENCODING, HttpHeaderValues.DEFLATE);

            pendingRequests.put(streamId, new PendingReq<>(responseTransformer, future, System.currentTimeMillis(), (timeoutMs > 0) ? System.currentTimeMillis() + timeoutMs : -1));
            channel.writeAndFlush(req, channel.voidPromise());
        });

}

Netty version

4.1.70.Final

JVM version (e.g. java -version)

java version "1.8.0_271"
Java(TM) SE Runtime Environment (build 1.8.0_271-b09)
Java HotSpot(TM) 64-Bit Server VM (build 25.271-b09, mixed mode)

OS version (e.g. uname -a)

Darwin sklas-mac 19.6.0 Darwin Kernel Version 19.6.0: Sun Nov 14 19:58:51 PST 2021; root:xnu-6153.141.50~1/RELEASE_X86_64 x86_64

@hyperxpro
Copy link
Contributor

Can you do a PR?

@alecharmon
Copy link
Contributor

alecharmon commented Jul 12, 2023

@hyperxpro Created a pr to address the issue @stefanklas was running into - Always increment Stream Id on createStream

@normanmaurer normanmaurer added this to the 4.1.95.Final milestone Jul 12, 2023
alecharmon added a commit to alecharmon/netty that referenced this issue Jul 18, 2023
Motivation:

When the creation of a stream causes an error for whatever reason we want to increment the next expected stream id.
ie: checkNewStreamAllowed raises an error which causes the headers frame to get rejected subsequently a data frame arrives and it throws a protocol error.

Modification:

Use a finally block so that we always increment the expected next stream id

Result:

Fixes netty#12065
@normanmaurer
Copy link
Member

BTW I think PROTOCOL_ERROR is also fine here: https://httpwg.org/specs/rfc7540.html#rfc.section.5.1.2

normanmaurer added a commit that referenced this issue Jul 19, 2023
Motivation:

When the creation of a stream causes an error for whatever reason we
want to increment the next expected stream id.
ie: checkNewStreamAllowed raises an error which causes the headers frame
to get rejected subsequently a data frame arrives and it throws a
protocol error.

Modification:

Use a finally block so that we always increment the expected next stream
id

Result:

Fixes #12065

---------

Co-authored-by: Norman Maurer <norman_maurer@apple.com>
normanmaurer added a commit that referenced this issue Jul 19, 2023
Motivation:

When the creation of a stream causes an error for whatever reason we
want to increment the next expected stream id.
ie: checkNewStreamAllowed raises an error which causes the headers frame
to get rejected subsequently a data frame arrives and it throws a
protocol error.

Modification:

Use a finally block so that we always increment the expected next stream
id

Result:

Fixes #12065

---------

Co-authored-by: Norman Maurer <norman_maurer@apple.com>
@idelpivnitskiy
Copy link
Member

Heads up that the original PR #13485 was reverted in Netty 4.1.96: 013c382

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.

5 participants