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/5.0] Split lock to prevent deadlock in Http2Stream. #47921

Merged
merged 2 commits into from
Mar 11, 2021

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Feb 5, 2021

Backport of #47769 to release/5.0
Fixes #48220
Deadlock within HTTP/2.

/cc @ManickaP

Customer Impact

Reported by Notification Hubs Azure team after an upgrade to .NET 5.0

Regression?

Based on code investigation it has been present in .NET Core 3.1.
However, the customer didn't observer the problem on .NET Core 3.1. This might be due to the fact they had much lower load before moving to .NET 5.0.

Testing

  • Local reproduction of the problem and confirmation of the fix.
  • The customer ran privately built bits (from this very PR) in production and confirmed the issue being fixed as well.

Risk

Low

ManickaP and others added 2 commits February 5, 2021 18:11
Http2Connection.ChangeInitialWindowSize locks connection's SyncObject and calls Http2Stream.OnWindowUpdate which locks stream's SyncObject.
Http2Stream.Complete is called only while stream's SyncObject lock is take and then it calls Http2Connection.RemoveStream that locks connection SyncObject.
…andler/Http2Stream.cs

Co-authored-by: Stephen Toub <stoub@microsoft.com>
@ghost
Copy link

ghost commented Feb 5, 2021

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

Issue Details

Backport of #47769 to release/5.0

/cc @ManickaP

Customer Impact

Testing

Risk

Author: github-actions[bot]
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@ManickaP ManickaP added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 5, 2021
@ManickaP ManickaP marked this pull request as draft February 5, 2021 18:14
@ManickaP ManickaP removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 12, 2021
@ManickaP ManickaP marked this pull request as ready for review February 12, 2021 11:05
@ManickaP ManickaP requested a review from a team February 12, 2021 11:05
@ManickaP ManickaP added the Servicing-consider Issue for next servicing release review label Feb 12, 2021
Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

Approved. We will take for 5.0.x consideration

@jeffschwMSFT jeffschwMSFT removed the Servicing-consider Issue for next servicing release review label Feb 18, 2021
@jeffschwMSFT jeffschwMSFT added this to the 5.0.x milestone Feb 18, 2021
@Anipik Anipik added Servicing-approved Approved for servicing release and removed Servicing-approved Approved for servicing release labels Mar 10, 2021
@jeffschwMSFT jeffschwMSFT added the Servicing-approved Approved for servicing release label Mar 11, 2021
@jeffschwMSFT
Copy link
Member

@Anipik this was approved in email. Can we merge?

@Anipik Anipik merged commit 7b5173f into release/5.0 Mar 11, 2021
@ManickaP ManickaP mentioned this pull request Mar 12, 2021
@jkotas jkotas deleted the backport/pr-47769-to-release/5.0 branch March 19, 2021 03:11
@danmoseley danmoseley modified the milestones: 5.0.x, 5.0.5 Apr 1, 2021
@ghost ghost locked as resolved and limited conversation to collaborators May 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Http Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants