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

Fix a couple of issues related to Process.WaitForExit on Unix #87174

Merged
merged 9 commits into from
Jun 20, 2023

Conversation

stephentoub
Copy link
Member

Fixes #85717

  • Calling WaitForExit(timeout), if the timeout occurred before the background work item launch, WaitForExit would throw an AggregateException wrapping an OperationCanceledException, rather than just returning false.
  • In rare race conditions, the background worker could end up overwriting with null the Task being used to track that there was a background operation. This could lead to there being a background Task in flight but it not actually being tracked anywhere, which then meant that we could end up with multiple polling loops all running concurrently. There was also an erroneous assert as a result (if WaitForExitAsync was called as part of a continuation, the lock that was supposed to be protecting it wouldn't be held).

@ghost
Copy link

ghost commented Jun 6, 2023

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

Issue Details

Fixes #85717

  • Calling WaitForExit(timeout), if the timeout occurred before the background work item launch, WaitForExit would throw an AggregateException wrapping an OperationCanceledException, rather than just returning false.
  • In rare race conditions, the background worker could end up overwriting with null the Task being used to track that there was a background operation. This could lead to there being a background Task in flight but it not actually being tracked anywhere, which then meant that we could end up with multiple polling loops all running concurrently. There was also an erroneous assert as a result (if WaitForExitAsync was called as part of a continuation, the lock that was supposed to be protecting it wouldn't be held).
Author: stephentoub
Assignees: -
Labels:

area-System.Diagnostics.Process

Milestone: -

@skyoxZ
Copy link
Contributor

skyoxZ commented Jun 6, 2023

/// <summary>If a wait operation is in progress, the Task that represents it; otherwise, null.</summary>
private Task? _waitInProgress;

The comments need to be updated.

@tmds
Copy link
Member

tmds commented Jun 7, 2023

@stephentoub I'll take a closer look tomorrow.

@tmds
Copy link
Member

tmds commented Jun 9, 2023

I'm unable to close the comments that have been resolved: #87174 (review), #87174 (review) and #87174 (review). Do you get a button for closing those? If so, you can close them?

I've added one new comment: #87174 (review).

Besides that suggestion, this looks good to me.

@stephentoub
Copy link
Member Author

Feedback addressed.

Copy link
Member

@jozkee jozkee left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@jozkee jozkee merged commit 7f4334e into dotnet:main Jun 20, 2023
@stephentoub stephentoub deleted the fixwaitforexitunix branch June 29, 2023 14:31
@ghost ghost locked as resolved and limited conversation to collaborators Jul 29, 2023
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.

Process.WaitForExit(Int32) sometimes throws AggregateException on macOS
4 participants