-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
API Proposal: WaitForExitAsync for System.Diagnostics.Process #28458
Comments
@krwq, Thanks for tagging this! Is there anything else you need on my end for the API review? |
@MattKotsenas, I don't believe so. We only need @terrajobst to pick this us during the API review. @terrajobst, any ETA on that? |
Hello again! Another friendly ping here. If there's an ETA for review please let me know so I can plan accordingly. Thanks! |
@krwq you will have to talk to Immo directly probably |
I'm also waiting on my own issues to get reviewed and have talked with him couple of times, perhaps we should have more API reviews.. - will talk to him about it |
This is not a high-pri API (it is in Future, not in 3.0). It may need to wait couple of more weeks (or months) before we get to it ... just trying to set expectations. |
Any updates on this? I'd really love to see this API in .NET Core. IMO it's one of the main pieces missing from the API. |
@Daniel15: API review is being done oldest to newest, and it looks like this is pretty near the top so I'd expect the wait to not be too much longer. |
Would it be a good idea to also add a timeout parameter? Often, that's all you want. You don't want to deal with creating a token just for that call.
|
Looks good as proposed. namespace System.Diagnostics
{
public partial class Process
{
public Task WaitForExitAsync(CancellationToken cancellationToken = default);
}
} |
@terrajobst @krwq @MattKotsenas |
@felipepessoto not sure I understand, where is the PR? |
I'm so happy this is approved 😄 |
@krwq, sorry, it is not a PR: dotnet/corefx@master...MattKotsenas:feature/34689-process-waitforexitasync |
@krwq, I'm happy to open a PR for this based on my branch. With the new dotnet runtime repo should I move this code over to there, or is there some more automated process? |
@MattKotsenas unfortunatelly you will need to move it, hopefully this will be just copy & paste since folder and file structure is almost the same (one extra subdirectory) but I'm not sure if there were some changes made there recently |
This was implemented in #1278. |
Per discussion with @krwq, I'm splitting off an API proposal for a Task-based
WaitForExitAsync
method onSystem.Diagnostics.Process
, since it is a building block API that is simpler to understand and use correctly, while the original issue (#12039) can continue to incubate additional convenience APIs.Here's the proposed API:
The method takes a single, optional parameter, a
CancellationToken
, to support timeouts, which matchesWaitForExit(int timeout)
semantics, while following Task-based programming best practices of allowing cancellation.API Rationale / Design notes
API doesn't expose an
WaitForExitAsync(int timeout)
overload sinceCancellationToken
has a constructor that takes a timeout, and there'sCancellationToken.TimeoutAfter()
.While the synchronous
WaitForExit
returns abool
to determine if the process exited or not, the async version returns a plainTask
. Callers can determine that the process exited in two ways:IsCompletedSuccessfully
HasExited
If the wait is cancelled, the caller can determine that in two ways:
await
, the task will throw aTaskCanceledException
IsCanceled
Callers that want to emulate the old API more closely can add an extension method as follows:
This API isn't provided by the framework because:
Because the method internally relies on the
Exited
event and there's a potential race between settingEnableRaisingEvents
and the process exiting, I introduced a new instance ofInvalidOperationException
informing the caller to setEnableRaisingEvents
to make this explicit. If this is deemed undesirable coupling I can move the set into this method, at the expense of possibly throwing and catching theInvalidOperationException
fromGetProcessHandle()
, which I'd rather avoid if possible.Implementation
I have an implementation available here with tests to show sample usage: feature/34689-process-waitforexitasync.
The text was updated successfully, but these errors were encountered: