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/6.0] Fix QUIC ConnectionState NRE in HandleEventConnectionClose #57742

Merged
merged 2 commits into from
Aug 23, 2021

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Aug 19, 2021

Backport of #57655 to release/6.0

Moved ConnectionState assignment before msquic callback registration to avoid NRE in callback, in case Connection gets closed during Stream's ctor.

Fixes #55815

/cc @CarnaViire

Customer Impact

It started hurting our CI on 8/17 badly: #55815 (comment)

  • Failures in main branch: 8/17 (11x failures), 8/18 (2x failures)
  • Failures in release/6.0 branches: 8/19 (2x failures), 8/20 (4 x failures)

Testing

CI passed. No new failures in main since merging #57655 (failures in release/6.0 still happen)
The issue is race condition, hard to test in any other way.

Risk

Low, QUIC and H/3 are hidden behind AppContext switch

@ghost
Copy link

ghost commented Aug 19, 2021

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

Issue Details

Backport of #57655 to release/6.0

/cc @CarnaViire

Customer Impact

Testing

Risk

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

area-System.Net

Milestone: -

@karelz karelz added this to the 6.0.0 milestone Aug 19, 2021
@ViktorHofer
Copy link
Member

@karelz is this ready to be merged (after looking at the one failure in the PR)? Asking as I noticed failures because of this in unrelated PRs targeting release/6.0.

@CarnaViire
Copy link
Member

runtime pipeline failure is NuGet restore, rerunning.
runtime-staging failures are in System.Net.WebSockets.Client.Tests, System.ComponentModel.Primitives.Tests, System.Security.Cryptography.Pkcs.Tests -- unrelated to this PR, rerunning.

@ViktorHofer
Copy link
Member

ViktorHofer commented Aug 20, 2021

runtime-staging failures are in System.Net.WebSockets.Client.Tests, System.ComponentModel.Primitives.Tests, System.Security.Cryptography.Pkcs.Tests -- unrelated to this PR, rerunning.

@CarnaViire when seeing unrelated failures, can you please link to their corresponding issues (and if such don't exist yet, open one and link to it)? That makes sure that all failures are tracked.

For more info see https://github.com/dotnet/runtime/blob/main/docs/pr-guide.md#option-2-there-is-a-flaky-test-that-is-not-related-to-your-pr

@CarnaViire
Copy link
Member

CarnaViire commented Aug 20, 2021

Unrelated failures in runtime-staging pipeline are:
System.Net.WebSockets.Client.Tests - wasm - seems to be #57519 (now closed without resolution, possibly tracked in #53957 instead? UPD 8/23: reopened)
System.ComponentModel.Primitives.Tests - Android - created #57821 (UPD 8/23: fixed and closed)
Other failures are crashes in iOS/tvOS/MacCatalyst - seem to be tracked by #53624

@karelz karelz requested a review from danmoseley August 23, 2021 14:21
@danmoseley
Copy link
Member

Approved for release/6.0 -- CI impact, localized change.

@danmoseley danmoseley merged commit 740581c into release/6.0 Aug 23, 2021
@danmoseley danmoseley deleted the backport/pr-57655-to-release/6.0 branch August 23, 2021 15:09
@ghost ghost locked as resolved and limited conversation to collaborators Sep 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants