Skip to content

Fix Http2Connection.GetIdleTicks - #55820

Merged
alnikola merged 1 commit into
dotnet:mainfrom
alnikola:alnikola/43877-fix-h2-getidleticks
Jul 16, 2021
Merged

Fix Http2Connection.GetIdleTicks#55820
alnikola merged 1 commit into
dotnet:mainfrom
alnikola:alnikola/43877-fix-h2-getidleticks

Conversation

@alnikola

@alnikolaalnikola commented Jul 16, 2021

Copy link
Copy Markdown
Contributor

There is currently a wrong operand order in the subtraction.

Fixes#43877

@alnikola
alnikola requested a review from a teamJuly 16, 2021 13:07
@ghost

ghost commented Jul 16, 2021

Copy link
Copy Markdown

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

alnikola commented Jul 16, 2021

Copy link
Copy Markdown
ContributorAuthor

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines

azure-pipelinesBot commented Jul 16, 2021

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

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

geoffkizer commented Jul 19, 2021

Copy link
Copy Markdown
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

alnikola commented Jul 20, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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.

@ManickaPManickaP mentioned this pull request Jul 20, 2021
Merged
@karelzkarelz added this to the 6.0.0 milestone Jul 22, 2021
@bartonjsbartonjs mentioned this pull request Jul 23, 2021
Closed
@ghostghost locked as resolved and limited conversation to collaborators Aug 21, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoubstephentoubstephentoub approved these changes
@karelzkarelzkarelz approved these changes
@ManickaPManickaPManickaP 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