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

[release/9.0-staging] Bugfix InvalidOperationException/IndexOutOfRangeException in HttpListener.EndGetContext #110695

Merged

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Dec 13, 2024

Backport of #107804 to release/9.0-staging
Fixes #107025

/cc @rokonec @pjannesen

Customer Impact

Reported by customer in #107025. Affects their admin access to services - causes reliability problem.

Regression

From .NET Framework

Testing

Note: The problem is intermittent and hard to reproduce.
Manual testing has been performed - customer validated with private patch prior to submitting .NET 10 PR - see #107025 (comment)

Risk

Low - these simple changes do not add or modify complex logic

…ust have exclusive access
Copy link
Contributor

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

Copy link
Member

@karelz karelz left a comment

Choose a reason for hiding this comment

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

Looks like we could have used DisconnectResults directly, there was no need to put it into a variable - that is a cosmetic change.
The real meaningful change is protecting the dictionary from unlocked access - which happened in 2 places.

Copy link
Member

@rokonec rokonec left a comment

Choose a reason for hiding this comment

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

I think it would would work even without using local variable disconnectResults however using it will not harm anything and could have slightly better runtime performance by not calling DisconnectResults getter multiple time.

Changes looks safe and correct to me.

@karelz
Copy link
Member

karelz commented Dec 19, 2024

It is worth fixing in servicing to unblock customers. Marking as such.

@karelz karelz added the Servicing-consider Issue for next servicing release review label Dec 19, 2024
@pjannesen
Copy link
Contributor

I think it would would work even without using local variable disconnectResults however using it will not harm anything and could have slightly better runtime performance by not calling DisconnectResults getter multiple time.

The reason behind the local variable is that the getter results in a Volatile.Read. Which makes it more expensive.

…/9.0-staging
@karelz
Copy link
Member

karelz commented Jan 10, 2025

Approved by .NET Shiproom (@SteveMCarroll) over email on 1/8. Marking Servicing-approved.

@karelz karelz added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jan 10, 2025
@rokonec
Copy link
Member

rokonec commented Jan 10, 2025

/ba-g failing flaky tests most probably unrelated to changes of this PR

@rokonec rokonec merged commit 56501c9 into release/9.0-staging Jan 10, 2025
82 of 84 checks passed
@rokonec rokonec deleted the backport/pr-107804-to-release/9.0-staging branch January 10, 2025 19:14
@github-actions github-actions bot locked and limited conversation to collaborators Feb 10, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants