Attaching to already running process

[konrad-kruczynski]

The idea is to extend curent Command.Run API by adding:

public static Command AttachTo(int processId)

which would internally use Process.GetProcessById.

What do you think?

[madelson]

@konrad-kruczynski thanks for your suggestion!

It would be helpful to me to be able to understand your use-case a bit better. One of the reasons I've held off creating an API like this so far is because, to my knowledge, you can't use the standard IO streams of a process fetched with GetProcessById(). For example:

var cmd = Process.Start(new ProcessStartInfo { FileName = "cmd.exe", RedirectStandardInput = true, RedirectStandardOutput = true, UseShellExecute = false });
cmd.Id.Dump();
cmd.StandardInput.WriteLine("echo hi"); // ok
Thread.Sleep(1000);

var proc2 = Process.GetProcessById(cmd.Id);
proc2.StandardInput.WriteLine("echo  bye"); // throws: standard in has not been redirected

What would you be trying to do with this API and would it work given that restriction (and potentially others)?

[konrad-kruczynski]

Indeed you can't redirect input/output, but operations like killing, getting priority, waiting for exit are still possible. My use case briefly:

• application starts some long-running processes and waits for their exit;
• sometimes application has to be restarted when processes are still running; in such case I'd like to store processes ids and reattach to them after restart.

Having this in MedallionShell would allow me to use common API for both.

[konrad-kruczynski]

I'd also like to check for the exit code of the attached process which is also possible if you access its SafeHandle, an example here.

[madelson]

Thanks @konrad-kruczynski . This is making me think about what value MedallionShell will add on top of the base Process API in this scenario. I'm imagining that actually the key benefit would be in dealing with the various race conditions inherent in this scenario. For example:

• What happens if the process dies between determining it's ID and trying to make a command for it (this one is tricky, because unfortunately we can't distinguish this from the case where the ID is just wrong)?
• What happens if the process dies just before we call Kill() (already handled by MedallionShell today)
• What happens if the process dies just before we subscribe to its HasExited event?
• Using the SafeHandle trick in the linked post (and handling the race condition where the process exits between fetching the process and accessing the handle) to keep process exit information around after exit
• Ability to associate the process with a timeout value or cancellation token to kill it when it dies

Having gone through the exercise, there's actually a decent amount here. What do you think? Am I missing anything? Would this be helpful?

For consistency, I'm imagining an API like:

public class Command
{
    public static bool TryAttachToProcessId(int processId, out Command command);
}

The reason I want this to be a Try operation is that in the case where the process does not exist, or exits before we can grab the safe handle, we can't really return a functional Command. The idea is that if you get a Command it will be guaranteed to have basic functionality like an exit code.

One open question is what options we should allow for such an API. Many of the options provided for Command.Run don't make sense in this context, but some (timeout, cancellationToken, throwOnError) DO make sense. Probably we'll take a separate Action<AttachToProcessOptionsBuilder> for this API.

Relatedly, should we add this to the Shell API? Probably for consistency, although likely it wouldn't be used very often.

Thoughts?

[konrad-kruczynski]

Having gone through the exercise, there's actually a decent amount here. What do you think? Am I missing anything? Would this be helpful?

Yes, exactly. For me even having a common API is valuable enough, but resolving all this corner cases is much better.

For consistency, I'm imagining an API like:

public class Command
{
    public static bool TryAttachToProcessId(int processId, out Command command);
}

Although in my initial proposition I proposed different version, just a little bit later I've realized that the try version would be the best option, so you've actually taken words from my mouth. One could also think to return enum instead of bool with possible values:

enum AttachingResult
{
    Success, // obvious
    AttachedWithoutHandle, //  exit code will not be available
    Failure // obvious
}

One open question is what options we should allow for such an API. Many of the options provided for Command.Run don't make sense in this context, but some (timeout, cancellationToken, throwOnError) DO make sense. Probably we'll take a separate Action<AttachToProcessOptionsBuilder> for this API.

Good point.

Relatedly, should we add this to the Shell API? Probably for consistency, although likely it wouldn't be used very often.

Same thoughts.

[madelson]

Thanks @konrad-kruczynski . I think this makes sense to add as part of the next release. Let me know if this is a feature you would be interested in contributing and I can provide some guidance on that front. Otherwise, I'll work on this in the next month or so.

[konrad-kruczynski]

I think I would be able to work on this in March. What do you think?

[madelson]

@konrad-kruczynski sounds great! Based on our previous discussion, here's where I think we should end up API-wise:

public sealed class Shell
{
    public static bool TryAttachToProcess(int processId, out Command command);
    public static bool TryAttachToProcess(int processId, Action<Options> options, out Command command);
}

public abstract class Command
{
    // these just call through to the same method on Shell.Default
    public static bool TryAttachToProcess(int processId, out Command command);
    public static bool TryAttachToProcess(int processId, Action<Shell.Options> options, out Command command);
}

Some notes:

• I thought about the enum vs. bool approach for the return value, and decided to go with bool because (a) it makes the usage pattern for the API simpler and (b) I couldn't think of a case where the consumer wanted to differentiate between the different failure states, since which one you end up in is purely a race condition.
• I know that I had originally proposed making a separate Options class, but in actuality the only options that don't make sense are StartInfo, Encoding, and CommandLineSyntax and of those really only StartInfo and Encoding seem like they should result in a thrown ArgumentException when attaching. Therefore, I think using the existing Shell.Options makes the most sense.

For implementation, I think the best route will be to introduce a new internal class which derives from Command called AttachedCommand. Implementing the abstract methods of Command should be pretty easy; for example the Std IO properties should throw InvalidOperationException with the same error message as in ProcessCommand for non-redirected streams.

As far as tests to add, it will be tricky to test every single possible race condition but I think that's ok. At minimum we should test:

• That all Std IO streams throw and that setting StartInfo or Encoding options throw.
• That Process/Processes are only available if DisposeOnExit is set to false
• That attaching to a completely bogus process ID returns false
• That attaching to an exited (but otherwise valid) process ID returns false
• That the Timeout option works as expected (it feels like this should be applied relative to Process.StartTime, but curious what you think people would expect the behavior to be).

When you're ready to file a pull request, please target the release-1.6 branch. Feel free to ask any questions/continue discussion on this thread, and also don't hesitate to let me know if you decide you won't be able to push on this.

[konrad-kruczynski]

Ok, thanks for all the information, I'll keep you updated about the progress.

[konrad-kruczynski]

I have finally started working on this. Stub of the work can be seen here: https://github.com/konrad-kruczynski/MedallionShell/tree/attached_command

Nothing actually interesting, maybe apart from the fact that .NET 4.5 does not have SafeHandle property. It, however, does have Handle property which seems to have the same features regarding side effects we're interested in.

Now, however, with Kill() and Task I'd naturally like to reuse the existing machinery from ProcessCommand, especially because the necessary methods are static. What would you prefer? Moving them up the class hierarchy (i.e. to the abstract Command) or maybe extracting them to some helper class?

[madelson]

Thanks!

.NET 4.5 does not have SafeHandle property

Interesting. It does make me a bit worried since Process doesn't have a finalizer and therefore if people forget to Dispose() (not unlikely because likely they are turning off DisposeOnExit() here since they want to get at WorkingSet or other process info) then the handle will leak. If we add a net46 build target, then we will at least prevent .NET 4.6 and 4.7 consumers from facing this, which seems worthwhile.

Also, while investigating this I ran into the fact that sometimes accessing the handle will throw Win32Exception with "access denied. You can see this by running foreach (var p in Process.GetProcesses()) { Console.WriteLine(p.Handle); p.Dispose(); }. We should think about options for handling this; likely we should just bubble this up with a nicer exception type and message and not allow you to attach to processes where you can't get the exit code. I wonder if the error is different than Win32Exception on other platforms? Probably worth a bit of investigation. It looks like this property can also throw NotSupportedException (see https://docs.microsoft.com/en-us/dotnet/api/system.diagnostics.process.handle?view=netframework-4.7.2), so that's another one to watch for. Maybe the right thing to wrap and rethrow all exceptions EXCEPT the InvalidOperationException that gets thrown when the process has already exited.

Now, however, with Kill() and Task I'd naturally like to reuse the existing machinery from ProcessCommand, especially because the necessary methods are static.

Moving these to an internal static ProcessHelper class makes sense to me.

[konrad-kruczynski]

Interesting. It does make me a bit worried since Process doesn't have a finalizer and therefore if people forget to Dispose() (not unlikely because likely they are turning off DisposeOnExit() here since they want to get at WorkingSet or other process info) then the handle will leak. If we add a net46 build target, then we will at least prevent .NET 4.6 and 4.7 consumers from facing this, which seems worthwhile.

However .NET 4.6/4.7 users can target .NET Standard 1.3, is that right? Different question is if they will ;)

Also, while investigating this I ran into the fact that sometimes accessing the handle will throw Win32Exception with "access denied. You can see this by running foreach (var p in Process.GetProcesses()) { Console.WriteLine(p.Handle); p.Dispose(); }. We should think about options for handling this; likely we should just bubble this up with a nicer exception type and message and not allow you to attach to processes where you can't get the exit code. I wonder if the error is different than Win32Exception on other platforms?

I've tested it on Debian, and it didn't show up (not running as root), so probably it's not a problem there. I can later test it on MacOS.

Probably worth a bit of investigation. It looks like this property can also throw NotSupportedException (see https://docs.microsoft.com/en-us/dotnet/api/system.diagnostics.process.handle?view=netframework-4.7.2), so that's another one to watch for. Maybe the right thing to wrap and rethrow all exceptions EXCEPT the InvalidOperationException that gets thrown when the process has already exited.

Good point, I'll do that.

Moving these to an internal static ProcessHelper class makes sense to me.

OK.

[konrad-kruczynski]

Since I have added a pull request, I think that we can close the issue and start the review process, maybe moving some part of the discussion there. What do you think?

[madelson]

@konrad-kruczynski now that the PR is linked to the issue I think the issue will close automatically when the PR is complete. Agreed that further discussion should happen on the PR

[madelson]

FYI @konrad-kruczynski I just published version 1.6 to NuGet. Apologies for the long delay; I was trying to bundle in some other improvements in the release which took longer than expected.

[konrad-kruczynski]

The delay was actually not a problem, in fact now is the time I'll start using the new feature. Thanks for the whole patch process, it (cooperation) was a pleasure.