3

I'm redesigning one of my programs which performs certain actions on processes of interest (known as "Monitored Processes" in my program).

Some actions I always need to do on those processes are:

  • Open process handle
  • Get process name
  • Close process handle

And some further actions I may need to do on some or all of them are:

  • Suspend process
  • Resume process
  • Set process priority
  • Set process affinity

Previously I simply had a "handler" class that opened a handle to the process, checked its name, and performed the requested actions. All of that is done via p/invoke, I don't use .NET's Process class at all.

However I am trying to redesign it from a SOLID perspective, so I created a "MonitoredProcess" object and have added a number of methods to it to perform the actions.

Here it is in its current state:

public class MonitoredProcess : IMonitoredProcess
{
    private readonly IMonitoredProcessConfig config;
    private readonly int processId;
    private IntPtr handle = IntPtr.Zero;
    private uint suspendResumeResult;
    private string processName;

    public IMonitoredProcessConfig Config { get { return config; } }
    public int ProcessId { get { return processId; } }
    public uint SuspendResumeResult { get { return suspendResumeResult; } }
    public string ProcessName
    {
        get
        {
            if (processName == null) PopulateProcessName();

            return processName;
        }
    }

    public MonitoredProcess(IMonitoredProcessConfig config, int processId)
    {
        if (config == null) throw new ArgumentNullException("config");

        this.config = config;
        this.processId = processId;
    }


    public bool OpenProcess()
    {
        var alreadyOpen = handle != IntPtr.Zero;
        if (alreadyOpen) return true;

        handle = Kernel32.OpenProcess(config.RequiredRights, false, (uint)processId);
        return handle != IntPtr.Zero;
    }

    public bool CloseProcess()
    {
        var closed = Kernel32.CloseHandle(handle);
        if (closed) handle = IntPtr.Zero;
        return closed;
    }

    public uint Resume()
    {
        suspendResumeResult = Ntdll.NtResumeProcess(handle);
        return suspendResumeResult;
    }

    public uint Suspend()
    {
        suspendResumeResult = Ntdll.NtSuspendProcess(handle);
        return suspendResumeResult;
    }

    private void PopulateProcessName()
    {
        var buffer = new StringBuilder((int)Kernel32.GeneralConstants.MAX_PATH);
        var success = 0 != Psapi.GetModuleFileNameEx(
            handle,
            IntPtr.Zero,
            buffer,
            (uint)buffer.Capacity
            );

        processName = success ? Path.GetFileName(buffer.ToString()) : string.Empty;
    }

}

I haven't yet added methods to set the process's priority and affinity. The interface it implements is not yet really defined, it will become whatever the class eventually exposes.

Question:

Is this class doing too much? Should a "process" know how to suspend/resume itself, and set its own priority and CPU affinity? Or should I have other classes that take an IMonitoredProcess and perform these actions on them (which is how I did it before)?


In the way I did it before, the classes performing the actions implemented them through p/invoke, so they "knew" they could throw and catch Win32Exceptions to get the error message. If this MonitoredProcess class implements everything via p/invoke internally, the class calling those methods technically shouldn't know how they're implemented, so has no reason to either throw or catch Win32Exceptions.

And I wouldn't want to simply throw them from this class (even wrapped in a domain-specific exception) because in half the places I call the methods I'm not interested in the outcome, but I'd still have to try/catch it every time - and it creates objects unnecessarily.

Which again makes me wonder if this class should really be doing all these actions, ostensibly on itself, but it uses external functions to perform all of them.

  • Much of what defines whether it is doing too much or two little depends on how its going to be used, and how it fits in with the system as a whole. – whatsisname Jan 15 '16 at 18:29
  • One change you should make is using a SafeHandle instead of a IntPtr. – CodesInChaos Jan 15 '16 at 19:42
4

I would call the class ProcessController rather then MonitoredProcess. The class instance is not a process, it encapsulates a process handle. And it does not just monitor, it controls.

You could pass the process id to the constructor.

This would get your terminology straight, it would be clear your class controls a process for you. Which is one responsibility. So you're good.

  • I quite like this. My naming of that class is definitely crappy, it probably reflects my unclear ideas. – 404 Jan 15 '16 at 21:31
  • Could also just flip the name to ProcessMonitor. – Corbin March Jan 16 '16 at 5:38
1

This depends largely on the context of a "MonitoredProcess" in your application as a whole.

Look at the name you've used: MonitoredProcess.

This implies that the process is being monitored by something. It may be a different class, method, or something else in your application that is monitoring your processes and deciding on whether or not it wants to perform these actions.

Based on that you have to determine if the monitor itself is more concerned with executing actions on processes or tracking results of actions on processes. In the former situation, it would be the monitor that wants to be able to perform the Open(), Close(), Resume(), and Suspend() operation on a process, not the MonitoredProcess that wants to perform these actions on itself. In the latter, the MonitoredProcess would want to contain these actions and the monitor would simply wants to invoke these operations on the process itself and manage the results.

1

The most useful way I've found to understand the Single Responsibility Principle is to interpret "responsibility" as "reason why the code might change". This is because the primary goal of SRP is to group together code that will change together, and (as far as possible) keep code that will change for different reasons separated from it. By doing this, we make it easier to make those changes if they're ever required.

In your case, the only reason you'd need to change your code is if the underlying system code that you're calling via p/invoke changes (either because a new version of Windows changes API in future, or if you decide to port your project to a different operating system).

From this point of view, your code is fine, although it may be worth (if you can ever foresee the possibility of an OS change) grouping this code together with any other system-dependent code, either in a subdirectory or in a library project.

The one change I would make is to encapsulate your return values in something more meaningful than a generic uint - as it stands, your clients need to understand the underlying Win32 implementation, which is a violation of the Law of Demeter. Create a specific return type that encapsulates the value returned by Windows.

  • Well indeed, and I was thinking that if I wanted to add new functionality to do something else with the process, I'd need to change the class and interface as well to expose a new method. Which is giving me doubts again whether this class should simply expose a process handle and allow other classes to perform actions on that handle (in which case new functionality would be implemented via a new class). – 404 Jan 16 '16 at 8:46
  • That would make things harder to change - you'd be scattering your system dependent code throughout your project. Keep it all together as tightly as you can. – Jules Jan 16 '16 at 8:50
  • I agree about the uints, however I don't know all the values the native methods can return (NTDLL stuff isn't really documented). I know of two return values (which account for all the cases I've seen so far) so I have defined constants for those, but I never know what the return value might be. – 404 Jan 16 '16 at 8:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.