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] Update ds-ipc-pal-namedpipe to fix shutdown race #89619

Merged
merged 1 commit into from
Aug 11, 2023

Conversation

hoyosjs
Copy link
Member

@hoyosjs hoyosjs commented Jul 28, 2023

Partial fix for #89621

Description

Works around a regression introduced in 6.0.20 around reading/writing to the IPC while VM shutdown is happening. The diagnostic server thread might race VM shutdown, and end up using an IPC handle that has been closed as part of a GetOverlappedResult call, that in turn will end up closing the handle.

The shutdown cleanup was added as a measure to ensure mono on Windows embedding scenarios would work after the EventPipe IPC got torn down.

Customer Impact

This can lead to hard to diagnose bugs on embedding scenarios where handle reuse is possible and the race will cause reused handles to be closed or odd reading behavior that's hard to diagnose. Currently EventPipe doesn't have any coordination mechanism during shutdown to make sure diagnostic server thread stops during shutdown, so it can't close any of the resources since they might still be used and their error paths lead to unexpected results.

Regression

Fixes regression introduced during last servicing.

Risk

Regresses scenario of embedding mono on Windows.

@hoyosjs hoyosjs requested a review from lateralusX July 28, 2023 09:04
@ghost ghost assigned hoyosjs Jul 28, 2023
@hoyosjs hoyosjs changed the title Update ds-ipc-pal-namedpipe to fix shutdown race [release/6.0] Update ds-ipc-pal-namedpipe to fix shutdown race Jul 28, 2023
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 consideration in 6.0.x

@jeffschwMSFT jeffschwMSFT added this to the 6.0.x milestone Jul 31, 2023
@carlossanlop
Copy link
Member

Friendly reminder: if you want this servicing fix to be included in the September 2023 Release, you'll have to merge this PR before August 14th.

@hoyosjs hoyosjs added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Aug 11, 2023
@hoyosjs hoyosjs merged commit ebdd264 into release/6.0-staging Aug 11, 2023
@hoyosjs hoyosjs deleted the juhoyosa/net6-ds-ipc-close-race branch August 11, 2023 09:02
@ghost ghost locked as resolved and limited conversation to collaborators Sep 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Tracing-coreclr Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants