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

HTTP/2 avoid closing connection when writing GOAWAY #9227

Merged
merged 1 commit into from Jun 7, 2019

Conversation

Projects
None yet
4 participants
@Scottmitch
Copy link
Member

commented Jun 6, 2019

Motivation:
b4e3c12 introduced code to avoid coupling
close() to graceful close. It also added some code which attempted to infer when
a graceful close was being done in writing of a GOAWAY to preserve the
"connection is closed when all streams are closed behavior" for the child
channel API. However the implementation was too overzealous and may preemptively
close the connection if there are not currently any open streams (and close if
there are any frames which create streams in flight).

Modifications:

  • Decouple writing a GOAWAY from trying to infer if a graceful close is being
    done and closing the connection. Even if we could enhance this logic (e.g.
    wait to close until the second GOAWAY with no error) it is possible the user
    doesn't want the connection to be closed yet. We can add a means for the codec
    to orchestrate the graceful close in the future (e.g. write some special "close
    the connection when all streams are closed") but for now we can just let the
    application handle this.

Result:
Fixes #9207

@Scottmitch Scottmitch added the defect label Jun 6, 2019

@Scottmitch Scottmitch added this to the 4.1.37.Final milestone Jun 6, 2019

@Scottmitch Scottmitch requested review from ejona86 and carl-mastrangelo Jun 6, 2019

@Scottmitch Scottmitch self-assigned this Jun 6, 2019

HTTP/2 avoid closing connection when writing GOAWAY
Motivation:
b4e3c12 introduced code to avoid coupling
close() to graceful close. It also added some code which attempted to infer when
a graceful close was being done in writing of a GOAWAY to preserve the
"connection is closed when all streams are closed behavior" for the child
channel API. However the implementation was too overzealous and may preemptively
close the connection if there are not currently any open streams (and close if
there are any frames which create streams in flight).

Modifications:
- Decouple writing a GOAWAY from trying to infer if a graceful close is being
  done and closing the connection. Even if we could enhance this logic (e.g.
wait to close until the second GOAWAY with no error) it is possible the user
doesn't want the connection to be closed yet. We can add a means for the codec
to orchestrate the graceful close in the future (e.g. write some special "close
the connection when all streams are closed") but for now we can just let the
application handle this.

Result:
Fixes #9207

@Scottmitch Scottmitch changed the title f HTTP/2 avoid closing connection when writing GOAWAY Jun 6, 2019

@Scottmitch Scottmitch force-pushed the Scottmitch:h2_goaway_no_close branch from 24c394d to 5afc095 Jun 6, 2019

@ejona86

ejona86 approved these changes Jun 6, 2019

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

@netty-bot test this please

@carl-mastrangelo
Copy link
Member

left a comment

LGTM

@Scottmitch Scottmitch merged commit 643d521 into netty:4.1 Jun 7, 2019

3 checks passed

pull request validation (centos6-java11) Build finished.
Details
pull request validation (centos6-java12) Build finished.
Details
pull request validation (centos6-java8) Build finished.
Details

@Scottmitch Scottmitch deleted the Scottmitch:h2_goaway_no_close branch Jun 7, 2019

Scottmitch added a commit that referenced this pull request Jun 7, 2019

HTTP/2 avoid closing connection when writing GOAWAY (#9227)
Motivation:
b4e3c12 introduced code to avoid coupling
close() to graceful close. It also added some code which attempted to infer when
a graceful close was being done in writing of a GOAWAY to preserve the
"connection is closed when all streams are closed behavior" for the child
channel API. However the implementation was too overzealous and may preemptively
close the connection if there are not currently any open streams (and close if
there are any frames which create streams in flight).

Modifications:
- Decouple writing a GOAWAY from trying to infer if a graceful close is being
  done and closing the connection. Even if we could enhance this logic (e.g.
wait to close until the second GOAWAY with no error) it is possible the user
doesn't want the connection to be closed yet. We can add a means for the codec
to orchestrate the graceful close in the future (e.g. write some special "close
the connection when all streams are closed") but for now we can just let the
application handle this.

Result:
Fixes #9207
@Scottmitch

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

master (e0bfc4f)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.