Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Do not directly abort connection on GOAWAY#39036

Merged
krwq merged 3 commits intodotnet:masterfrom
krwq:fix-39014
Jul 1, 2019
Merged

Do not directly abort connection on GOAWAY#39036
krwq merged 3 commits intodotnet:masterfrom
krwq:fix-39014

Conversation

@krwq
Copy link
Member

@krwq krwq commented Jun 28, 2019 •
edited
Loading

@krwq krwq requested a review from a team June 28, 2019 18:57
@davidsh davidsh added this to the 3.0 milestone Jun 28, 2019
@krwq krwq requested review from geoffkizer and stephentoub June 28, 2019 19:08
stephentoub
stephentoub approved these changes Jun 28, 2019
View reviewed changes
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments, but otherwise LGTM. @geoffkizer should review, too, though.

@geoffkizer
Copy link

geoffkizer commented Jun 28, 2019

BTW, this should also (hopefully) fix some related issues with GOAWAY. Since we are currently disposing when we receive GOAWAY, we disallow any frames from being sent after GOAWAY is received. This affect PING per #38558, but it also affects a whole bunch of other cases as well, such as:

(1) A request body may still be in flight, and as long as the GOAWAY last stream id indicates it's valid, we need to continue to send it
(2) Outbound WINDOW_UPDATE for response bodies
(3) SETTINGS ACK, if we receive a new SETTINGS frame (unlikely, but still valid, I think)

It would be nice to have tests for at least some of this, but it's not necessary as part of this PR.

@krwq
Copy link
Member Author

krwq commented Jul 1, 2019

@geoffkizer I've updated https://github.com/dotnet/corefx/issues/34638 to include your feedback:
#39036 (comment)

Please let me know if there is anything else I should address here or if we can merge this

@geoffkizer
Copy link

geoffkizer commented Jul 1, 2019

I'm happy, thanks.

@krwq krwq merged commit cc6435b into dotnet:master Jul 1, 2019
@geoffkizer geoffkizer mentioned this pull request Feb 1, 2020
Closed
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
@krwq
Do not directly abort connection on GOAWAY (dotnet/corefx#39036)
61de06b 
* Do not directly abort connection on GOAWAY

* Address feedback and add disabled GOAWAY tests

* remove redundant frame ignoring logic


Commit migrated from dotnet/corefx@cc6435b
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@stephentoub stephentoub stephentoub approved these changes

@geoffkizer geoffkizer geoffkizer approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

3.0

Development

Successfully merging this pull request may close these issues.

HTTP2: Abort streams on server disconnect after GOAWAY HTTP2: HttpClient does not respond to pings after GOAWAY when some streams are still active

4 participants

@krwq @geoffkizer @stephentoub @davidsh