Tyrrrz / CliWrap

Library for running command-line processes
MIT License
4.32k stars 264 forks source link

Wrong process ID if target executable doesn't exist #139

Closed alex6dj closed 2 years ago

alex6dj commented 2 years ago

Version

3.3.3

Details

When I try tu run an executable using this library, and the executable dont exist, it reports process id as zero. The problem is that the zero process id is valid and it represent the System Idle Process, it is not really a process but can cause a Win32Exception if you try to change his id process priority.

Steps to reproduce

This is part of my code

string ffmpegPath = "...";

var cmd = Cli.Wrap(ffmpegPath)
                .WithValidation(CommandResultValidation.None)
                .WithArguments(arguments);

var observable = cmd.Observe(cancellationToken).Publish();

var changePriority = observable.Select(x => x as StartedCommandEvent)
                .Where(x => x != null)
                .Take(1)
                .Select(x => x.ProcessId)
                //.Where(x => x > 0)   // This filters the process id only accepting values above zero
                .Subscribe(SetPriorityByProcessId); // Proccess id == 0  then Win32Exception

observable.Connect()

void SetPriorityByProcessId(int processId)
{
    var process = Process.GetProcessById(processId);
    process.PriorityClass = ProcessPriorityClass.BelowNormal;
}
alex6dj commented 2 years ago

I think we should avoid the use of process id zero or at least inform the behaviour.

Tyrrrz commented 2 years ago

If the target file path doesn't exist, it should throw an exception before it even produces StartedCommandEvent. Are you sure that's what's happening?

alex6dj commented 2 years ago

Yes, I am sure this is what is happening in my case. I am going to try to create a small project to replicate the error.

alex6dj commented 2 years ago

Project done. CliWrapWrongIdExample

alex6dj commented 2 years ago

This use latest version (3.4) and same behaviour.

Tyrrrz commented 2 years ago

Thanks I'll take a look

Tyrrrz commented 2 years ago

Thank you for the report, fixed. It should now throw an exception as expected.