Process.Start() failure should include path

[danmoseley]

When a process cannot be started, it would be useful for the exception to include the ProcessStartInfo or equivalent.

For example, I got this in the VS output window - I assume it's missing a certain dotnet.exe but I don't know which. I've encountered this in other situations.

Microsoft.VisualStudio.TestPlatform.ObjectModel.TestPlatformException: Failed to launch testhost with error: System.AggregateException: One or more errors occurred. ---> System.ComponentModel.Win32Exception: The system cannot find the file specified
   at System.Diagnostics.Process.StartWithCreateProcess(ProcessStartInfo startInfo)
   at System.Diagnostics.Process.Start()
   at Microsoft.VisualStudio.TestPlatform.PlatformAbstractions.ProcessHelper.LaunchProcess(String processPath, String arguments, String workingDirectory, IDictionary`2 envVariables, Action`2 errorCallback, Action`1 exitCallBack, Action`2 outputCallBack)
   at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Hosting.DotnetTestHostManager.LaunchHost(TestProcessStartInfo testHostStartInfo, CancellationToken cancellationToken)
   at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Hosting.DotnetTestHostManager.<>c__DisplayClass38_0.<LaunchTestHostAsync>b__0()
   at System.Threading.Tasks.Task`1.InnerInvoke()

We already special case two codes
https://github.com/danmosemsft/runtime/blob/58e6ab1054b8809b3c91fcd75d1dbfff559e9573/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs#L612-L616

We would special case ERROR_DIRECTORY, ERROR_FILE_NOT_FOUND, ERROR_INVALID_PARAMETER, ERROR_INVALID_NAME, ERROR_PATH_NOT_FOUND, and ERROR_ACCESS_DENIED and for those include psi.FileName and psi.WorkingDirectory in the message. (Or just do it in all cases) and analogous in Unix:

runtime/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs

Line 528 in 1923bba

throw new Win32Exception(errno);

[ghost]

Tagging subscribers to this area: @eiriktsarpalis
See info in area-owners.md if you want to be subscribed.

[danmoseley]

Likely something like

@@ -613,7 +613,9 @@ private unsafe bool StartWithCreateProcess(ProcessStartInfo startInfo)
                         {
                             throw new Win32Exception(errorCode, SR.InvalidApplication);
                         }
-                        throw new Win32Exception(errorCode);
+
+                        string msg = SR.Format(SR.FailedToStartFileDirectory, startInfo.FileName, startInfo.WorkingDirectory, new Win32Exception(errorCode).Message);
+                        throw new Win32Exception(errorCode, msg);

and

+  <data name="FailedToStartFileDirectory" xml:space="preserve">
+    <value>Failed to start '{0}' with working directory '{1}'. {2}</value>
+  </data>

[danmoseley]

dotnet/msbuild#5355 is another example where this would be useful.

[danmoseley]

@Symbai @batzen both of you thumbsed up - any interest in taking this one?

[batzen]

Sure. Will use some of the time i reserved for Hacktobest and try to implement this.

[danmoseley]

Sounds great to me - assigned to you.

[batzen]

I started to have a look at the code and was wondering how to solve this i an way that ensures that we always provide the information about file and working directory.
I guess it would be best to wrap StartCore with try/catch and throw a dedicated, new, exception (for example ProcessStartException). But i also guess that would be a massive, most likely, breaking change.
What do you think? @danmosemsft

A sample of such a possible solution (quick and dirty) can be found at https://github.com/batzen/runtime/tree/issues/39928

[danmoseley]

I’m not able to look at the code right now, but is a diff like the one I pasted above not possible?

[batzen]

Don't know how to produce a diff here.

But the could would be like:

public bool Start()
{
    Close();

    ProcessStartInfo startInfo = StartInfo;
    if (startInfo.FileName.Length == 0)
    {
        throw new InvalidOperationException(SR.FileNameMissing);
    }

    try
    {
        if (startInfo.StandardInputEncoding != null && !startInfo.RedirectStandardInput)
        {
            throw new InvalidOperationException(SR.StandardInputEncodingNotAllowed);
        }
        if (startInfo.StandardOutputEncoding != null && !startInfo.RedirectStandardOutput)
        {
            throw new InvalidOperationException(SR.StandardOutputEncodingNotAllowed);
        }
        if (startInfo.StandardErrorEncoding != null && !startInfo.RedirectStandardError)
        {
            throw new InvalidOperationException(SR.StandardErrorEncodingNotAllowed);
        }
        if (!string.IsNullOrEmpty(startInfo.Arguments) && startInfo.ArgumentList.Count > 0)
        {
            throw new InvalidOperationException(SR.ArgumentAndArgumentListInitialized);
        }

        //Cannot start a new process and store its handle if the object has been disposed, since finalization has been suppressed.
        if (_disposed)
        {
            throw new ObjectDisposedException(GetType().Name);
        }

        SerializationGuard.ThrowIfDeserializationInProgress("AllowProcessCreation", ref s_cachedSerializationSwitch);

        return StartCore(startInfo);
    }
    catch (Exception exception)
    {
        string workingDirectory = !string.IsNullOrWhiteSpace(startInfo.WorkingDirectory) ? startInfo.WorkingDirectory : Directory.GetCurrentDirectory();
        string msg = SR.Format(SR.FailedToStartFileDirectory, startInfo.FileName, workingDirectory);
        throw new ProcessStartException(msg, startInfo, exception);
    }
}

[Serializable]
public class ProcessStartException : Exception
{
    public ProcessStartException()
    {
    }

    public ProcessStartException(string message) 
        : base(message)
    {
    }

    public ProcessStartException(string message, ProcessStartInfo startInfo) 
        : base(message)
    {
        this.StartInfo = startInfo;
    }

    public ProcessStartException(string message, Exception inner) 
        : base(message, inner)
    {
    }

    public ProcessStartException(string message, ProcessStartInfo startInfo, Exception inner) 
        : base(message, inner)
    {
        this.StartInfo = startInfo;
    }

    protected ProcessStartException(SerializationInfo info, StreamingContext context) 
        : base(info, context)
    {
    }

    [field: NonSerialized]
    public ProcessStartInfo StartInfo { get; private set; }
}

[danmoseley]

Can’t you just adjust the existing message as I have shown above?

[batzen]

Of course.
Was just thinking a bit further. There are so many cases that could make a process fail to start where that info would be useful.
But i guess that should be part of a larger discussion.
Will implement a minimal solution like you suggested.

[danmoseley]

Sure, let's see how that works out in your testing. BTW, new API (like your ProcessStartException) has to go through an approval process which would consider many things such as whether it has sufficient value. In this case I think the exception type is OK.

BTW, this ugliness new Win32Exception(errorCode).Message suggests it might be useful to have an API to get a message from an error code without instantiating a dummy exception (assuming there isn't already a way that I missed). That's a separate issue to this PR though.

[danmoseley]

@batzen did you have success?

[batzen]

Not yet. Had get some other OSS stuff done first.
Will try to start this weekend.

[adamsitnik]

@batzen have you made any progress on this? do you need some help?

[batzen]

I had some massive issues with the build scripts.

• Build scripts used mixed windows sdk versions
• Build scripts used mixed visual studio versions (preview / non preview)
• Build scripts use the debug configuration by default which causes massive slowdowns, at least on my machine, and i wasn't able to get even one test to finish before hitting a timeout

I sorted all those out by "fixing" the build scripts on my machine. When i am done with my changes and the PR i will try to write all that down and create a separate issue for that.

According to my progress:
I made the changes, but i am not happy with creating an intermediate exception just to get the native error message and will have a look at how to get around that.
And i have to write a few new unit tests instead of just adjusting the existing ones.
My holidays start Wednesday, so i hope to get my PR done by the end of the week.

Sorry for the long delays but i am very busy at work as we are porting 1 million lines of code from vb6 to c# and that's a lot of work. ;-)

[danmoseley]

Build scripts use the debug configuration by default which causes massive slowdowns, at least on my machine, and i wasn't able to get even one test to finish before hitting a timeout

@batzen hmm, the steps I normally use for libraries work are essentially these
https://github.com/dotnet/runtime/blob/master/docs/workflow/building/libraries/README.md

build.cmd clr+libs -rc Release builds a release runtime - with debug libraries, that should run fast.

I'm not sure about the other two issues unfortunately. I do most of the build on the command line, in a regular command prompt (not a developer prompt) although I wouldn't expect that part to matter.

[batzen]

@adamsitnik @danmosemsft I could need some help.
While trying to make the tests culture agnostic i tried to use SR to get the error message, but the class is not accessible from the test project. I tried to figure out how it's included in the non test project, but couldn't find where it gets included/referenced.

As soon as that mystery is solved i can finish my changes. ;-)

[danmoseley]

@batzen our tests usually don't verify the exception string. We verify the exception type, and if looking for a particular value in the string (the path in this case, presumably) we just use this pattern
https://github.com/danmosemsft/runtime/blob/65dd3e206a3a717816aeddf644413ac6209fb153/src/libraries/System.Reflection/tests/AssemblyTests.cs#L372