Improved process wrap up on cancellation

[liiri]

• Only set cancellation source as cancelled when process had exit
• After killing process, wait with a timeout for actual termination
• If process termination failed, propagate the exception

[EamonNerbonne]

Hi! apparently this project is dozing slightly ;-)

I'm misssing the aim of this PR (which isn't to say it's a bad idea, just that I don't understand it!)

Why unregister event listeners before the process has really terminated? Why even give up after 30 seconds - I mean, a process that won't die after 30s is unfortunate, but how does a timeout help?

I'm guessing you have a battle-scar story of something that went wrong, inspiring this PR; I'd love to hear it - that would make it easier to review.

[liiri]

Hiya, this came from necassity - we had issues where process was not properly shut down after cancellation, and figured the library is setting the tcs before full verification that the process exited.

The unregister is to make sure process manages to exit. see for example https://stackoverflow.com/questions/139593/processstartinfo-hanging-on-waitforexit-why
For the same reason the Sleep was added, couldn't find its root cause, but without it, we got hangs on waiting for graceful exit.

I think if nothing else, 2fe3aef should be integrated as a bug fix (I can separate the PRs), because tcs.TrySetCanceled(); was called before process was actually killed. That for sure can cause a race condition.

[jamesmanning]

@liiri that SO question talks about the internal process buffer filling up, which shouldn't be happening here since we're doing the async reads from the output/error to prevent that. There may certainly be other deadlock conditions, but just wanted to make sure it was clear that this particular one shouldn't be happening, since it's caused by a misuse of the Process API (asking redirection but not listening)

@EamonNerbonne good point about the snoozing. 😄 The progress of CliWrap makes me wonder if we shouldn't just deprecate RunProcessAsTask in favor of it at this point? What do you think?

[EamonNerbonne]

The progress of CliWrap makes me wonder if we shouldn't just deprecate RunProcessAsTask in favor of it at this point? What do you think?

Wow, that looks pretty slick, indeed. I haven't tried it yet, but yeah, it looks like a complete replacement, pretty much?

[jamesmanning]

@EamonNerbonne yeah, I tried it out ~3.5 years ago when looking to see if anyone had something for listening to live events for stdout/stderr (and added a link to it in the README.md here since it worked great for that in my testing) but it's made enough progress at this point that I can't see any use cases left where RunProcessAsTask would be a better fit.

@liiri as a data point, would CliWrap work for you and does it handle your process exit/wrap up issues?

[EamonNerbonne]

It tries to handle this case, but it's fundamentally tricky:

https://github.com/Tyrrrz/CliWrap/blob/dfe92f33eb710d7aec529c0b425ead0be5db2b2a/CliWrap/Utils/ProcessEx.cs#L106

And that's a pretty nice solution too - just kill the proc, leave the exit-handler live to deal with the event of the actual process death, yet have a (short) timeout to abort further eventhandling when the process simply won't die. That's likely good enough to deal with most real delays.

[liiri]

@EamonNerbonne yeah, I tried it out ~3.5 years ago when looking to see if anyone had something for listening to live events for stdout/stderr (and added a link to it in the README.md here since it worked great for that in my testing) but it's made enough progress at this point that I can't see any use cases left where RunProcessAsTask would be a better fit.

@liiri as a data point, would CliWrap work for you and does it handle your process exit/wrap up issues?

Thanks, I'll check out the CiliWrap project