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.

Sign up for GitHub

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

Jump to bottom

Fix Http2Connection.GetIdleTicks #55820

Merged
merged 1 commit into from
Jul 16, 2021
Merged

Fix Http2Connection.GetIdleTicks #55820

merged 1 commit into from
Jul 16, 2021

Conversation

alnikola
Copy link
Contributor

There is currently a wrong operand order in the subtraction.

Fixes #43877

@alnikola alnikola requested a review from a team July 16, 2021 13:07
@ghost
Copy link

ghost commented Jul 16, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

There is currently a wrong operand order in the subtraction.

Fixes #43877

Author: alnikola
Assignees: -
Labels:

area-System.Net.Http
Milestone: -

@alnikola
Copy link
Contributor Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@alnikola alnikola merged commit 04a8f5e into dotnet:main Jul 16, 2021
@alnikola alnikola deleted the alnikola/43877-fix-h2-getidleticks branch July 16, 2021 15:13
@karelz karelz mentioned this pull request Jul 19, 2021
Closed
@geoffkizer
Copy link
Contributor

Wow, thanks for catching this.

Why didn't this surface sooner? I guess it's a weird one, because it would cause connections to be treated as idle when they weren't, but only if they had 0 active requests?

I'm trying to figure out how to guard against this in the future. Seems like we should at least assert somewhere that the result of GetIdleTicks() is >= 0?

@alnikola
Copy link
Contributor Author

alnikola commented Jul 20, 2021 •
edited
Loading

I guess it's a weird one, because it would cause connections to be treated as idle when they weren't, but only if they had 0 active requests?

It's really the opposite. In the broken version, a connection would never be treated as idle. This behavior failed the test that expected an idle connection to be removed.

@ManickaP ManickaP mentioned this pull request Jul 20, 2021
Merged
@karelz karelz added this to the 6.0.0 milestone Jul 22, 2021
@bartonjs bartonjs mentioned this pull request Jul 23, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Aug 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub approved these changes

@karelz karelz karelz approved these changes

@ManickaP ManickaP ManickaP approved these changes

Assignees
No one assigned
Labels
area-System.Net.Http
Projects
None yet
Milestone
6.0.0
5 participants
@alnikola @geoffkizer @stephentoub @karelz @ManickaP