IProcess WaitForExit(int) does not just observe whether the process has exited, it will kill a running process

[richardissimo]

Prerequisites

• [X] I have written a descriptive issue title
• [X] I have searched issues to ensure it has not already been reported

Cake runner

Cake .NET Tool

Cake version

4.0.0

Operating system

Windows

Operating system architecture

64-Bit

CI Server

No response

What are you seeing?

I have a cake build which launches multiple processes, using StartAndReturnProcess:

            var process = StartAndReturnProcess(runServiceBatPath.FullPath, settings);

I keep hold of those processes in a collection, and I have a loop which keeps an eye on those processes to see whether they are still running or not.

I am using "WaitForExit(1)" to see whether the process is still running or not, because there isn't a property on IProcess like the one on System.Diagnostics.Process called HasExited; so WaitForExit(int) should do the same thing.

But those processes seem to be mysteriously dying, which I've tracked down to being in the Cake code, if the process is still running, it kills the process. This is not only terrible (because there is no way of checking whether the process is still running); but fixing it will be a breaking change.

What is expected?

I would expect WaitForExit(1) to wait for up to a millisecond, before returning whether the process has exited or not (just like the documentation says). I do not expect it to kill the process.

Steps to Reproduce

The simplest way of replicating is to start any process that you know will remain running for a while - ideally one that you can see visually, or is easy to identify in Task Manager.

 var process = StartAndReturnProcess(yourProcessPathGoesHere);
 if (process.WaitForExit(1))
 {
     throw new Exception("Process is not running");
 }

When you run this, you will find that the WaitForExit has actually killed the process, rather than just observing whether it has exited or not.

Output log

No response

[richardissimo]

This was referred to 9 years ago, saying that it should probably be changed: https://github.com/cake-build/cake/issues/342#issuecomment-129929909

[devlead]

Having a timeout that kills process running longer than expected makes sense in many scenarios. Changing it would potentially break hundreds of thousands of builds and addins that depend on existing behavior, my suggestion would probably be to introduce an overload that has an keepAlive bool, settings class or similar.

[richardissimo]

@devlead As explained in the documentation for "WaitForExit(int)" the parameter is not a timeout relating to killing the process. It is the amount of time to wait for the process to exit before returning an answer - if the caller wanted to kill the process at that point, they could do so themselves by paying attention to the response and then (if the response was false) calling Kill().

There is no mention on the documentation for that method about killing the process. This method is clearly intended to represent the System.Diagnostics.Process.WaitForExit(int) functionality, which does not kill the process. So as part of the fix for this, the documentation for the IProcess method should be updated to explain this unexpected behaviour.

Clearly, making this change would be a breaking change (as I mentioned in the question), so if we're looking for a non-breaking solution to this, then adding an equivalent of the HasExited property to the IProcess would be perfect (that was what I looked for originally, before I settled on having to use WaitForExit(1)). Would that be acceptable?