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

Http2FrameCodec & Http2MultiplexHandler race condition #9432

Closed
Bennett-Lynch opened this issue Aug 6, 2019 · 1 comment · Fixed by #9442
Closed

Http2FrameCodec & Http2MultiplexHandler race condition #9432

Bennett-Lynch opened this issue Aug 6, 2019 · 1 comment · Fixed by #9442
Milestone

Comments

@Bennett-Lynch
Copy link
Contributor

Bennett-Lynch commented Aug 6, 2019

I am experiencing what appears to be a race condition as part of trying to migrate HTTP/2 client code from Http2MultiplexCodec to Http2FrameCodec/Http2MultiplexHandler, as was introduced/recommended in Netty 4.1.37.

As per the release notes, we are recommended to change:

pipeline.addLast(Http2MultiplexCodecBuilder.forServer(new HelloWorldHttp2Handler()).build());

to:

pipeline.addLast(Http2FrameCodecBuilder.forServer().build());
pipeline.addLast(new Http2MultiplexHandler(new HelloWorldHttp2Handler());

My client code is written in an event-driven fashion so that upon opening up a new HTTP/2 connection and detecting a Http2ConnectionPrefaceAndSettingsFrameWrittenEvent, it will open a new stream with Http2StreamChannelBootstrap#open(). This is the recommended usage of the API/event as per #7599 (comment).

Problem:

  1. Upon Http2FrameCodec being added to the pipeline, it may instantly fire a Http2ConnectionPrefaceAndSettingsFrameWrittenEvent (if the connection is already active).
  2. This triggers a call to Http2StreamChannelBootstrap#open(), before Http2MultiplexHandler has been added to the pipeline.
  3. Http2MultiplexHandler requires that Http2FrameCodec be present in the pipeline when adding it, so it cannot be added first.

So what used to be an atomic operation is no longer one, resulting in this apparent race condition. This does not appear to be an issue if the connection is not yet active, but there are some use cases where some type of logic/handshake might be performed before registering the HTTP/2 codecs.

I could potentially work around this by creating and firing my own user event for once both Http2FrameCodec and Http2MultiplexHandler have been added to the pipeline, but I believe I would need to await both that event and the Http2ConnectionPrefaceAndSettingsFrameWrittenEvent, in case the latter is not fired instantly for some reason. This also seems hacky and seems to defeat the purpose of the Http2ConnectionPrefaceAndSettingsFrameWrittenEvent.

@normanmaurer

normanmaurer added a commit that referenced this issue Aug 12, 2019
…Loop tick when using the Http2FrameCodec

Motivation:

We should delay the firing of the Http2ConnectionPrefaceAndSettingsFrameWrittenEvent by one EventLoop tick when using the Http2FrameCodec to ensure all handlers are added to the pipeline before the event is passed through it.

This is needed to workaround a race that could happen when the preface is send in handlerAdded(...) but a later handler wants to act on the event.

Modifications:

Offload firing of the event to the EventExecutor.

Result:

Fixes #9432.
@normanmaurer
Copy link
Member

@Bennett-Lynch #9442 PTAL

@normanmaurer normanmaurer added this to the 4.1.39.Final milestone Aug 12, 2019
normanmaurer added a commit that referenced this issue Aug 13, 2019
…Loop tick when using the Http2FrameCodec (#9442)

Motivation:

We should delay the firing of the Http2ConnectionPrefaceAndSettingsFrameWrittenEvent by one EventLoop tick when using the Http2FrameCodec to ensure all handlers are added to the pipeline before the event is passed through it.

This is needed to workaround a race that could happen when the preface is send in handlerAdded(...) but a later handler wants to act on the event.

Modifications:

Offload firing of the event to the EventExecutor.

Result:

Fixes #9432.
normanmaurer added a commit that referenced this issue Aug 13, 2019
…Loop tick when using the Http2FrameCodec (#9442)

Motivation:

We should delay the firing of the Http2ConnectionPrefaceAndSettingsFrameWrittenEvent by one EventLoop tick when using the Http2FrameCodec to ensure all handlers are added to the pipeline before the event is passed through it.

This is needed to workaround a race that could happen when the preface is send in handlerAdded(...) but a later handler wants to act on the event.

Modifications:

Offload firing of the event to the EventExecutor.

Result:

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