1
\$\begingroup\$

I previously posted a question: Single instancing class and received some good feedback on it.

I have implemented most of the changes suggested by @EBrown.

I also implemented the suggestion by @t3chb0t to go with SRP. The user also suggested removing regions but these are a coding standard where I work so I can't do that. Plus I actually like them... shhhhhh.

Lastly, I have added more functionality into the code to close an already running application instead of just showing one and implemented IDisposable. Implementing that interface, and using it properly, means that the mutex should now be getting released properly.


The InstanceManager class provides all the functionality to detect if an instance of the application is already running and then close/show the running application.

using System;
using System.Diagnostics;
using System.Reflection;
using System.Security.AccessControl;
using System.Security.Principal;
using System.Threading;

namespace CompanyName
{
    using Managers;
    using static Win32.User32;

    /// <summary>
    /// A class that provides functionality for single instancing an application.
    /// </summary>
    public sealed class InstanceManager : IDisposable
    {
        #region Fields

        private Guid _appGuid;

        private string _mutexId;
        private Mutex _mutex;
        private bool _isMutexOwner;

        #endregion

        #region Constructor

        /// <summary>
        /// Creates an instance of the class
        /// </summary>
        /// <param name="guid">The guid to override the used one with, if null is passed it uses the applications guid.</param>
        /// <param name="currentUserOnly">If true the manager will only check if the application is running under the current user.</param>
        public InstanceManager(Guid? guid = null, bool currentUserOnly = true)
        {
            //Is a .NET app so GetAssemblyGuid should always return a value for this process
            _appGuid = guid.HasValue ? guid.Value : AssemblyManager.GetAssemblyGuid(Assembly.GetEntryAssembly()).Value;

            _mutexId = $"{(currentUserOnly ? "" : "Global\\")}{{{_appGuid.ToString()}}}";
            CreateMutex();
        }

        #endregion

        #region Destructor

        /// <summary>
        /// Releases the mutex is the current application is the owner of it.
        /// </summary>
        public void Dispose()
        {
            if (_isMutexOwner)
            {
                _mutex.ReleaseMutex();
            }
        }

        #endregion

        #region PublicMethods

        /// <summary>
        /// Checks if the application is already running or not.
        /// </summary>
        /// <returns>A bool indicating whether or not the application is running.</returns>
        public bool IsInstanceRunning()
        {
            if (_isMutexOwner)
            {
                return false;
            }

            try
            {
                _isMutexOwner = _mutex.WaitOne(0, false);
                if (!_isMutexOwner)
                {
                    return true;
                }
            }
            catch (AbandonedMutexException)
            {
                _isMutexOwner = true;
            }

            return false;
        }

        /// <summary>
        /// Shows the running application with the same guid as the application that calls it.
        /// </summary>
        /// <returns>A bool indicating whether or not the instance was found and shown.</returns>
        public bool ShowRunningInstance()
        {
            Process process;
            if (ProcessManager.TryGetProcess(_appGuid, out process))
            {
                ShowProcess(process);
                return true;
            }

            return false;
        }

        /// <summary>
        /// Shows a running application from the supplied title parts.
        /// </summary>
        /// <param name="titleParts">An array of strings, all of which must be present in the title of the main window.</param>
        /// <returns>A bool indicating whether or not the instance was found and shown.</returns>
        public bool ShowRunningInstance(params string[] titleParts)
        {
            Process process;
            if (ProcessManager.TryGetProcess(out process, titleParts))
            {
                ShowProcess(process);
                return true;
            }

            return false;
        }

        /// <summary>
        /// Closes the running application with the same guid as the application that calls it.
        /// </summary>
        /// <returns>A bool indicating whether or not the instance was found and closed.</returns>
        public bool CloseRunningInstance()
        {
            Process process;
            if (ProcessManager.TryGetProcess(_appGuid, out process))
            {
                CloseProcess(process);
                return true;
            }

            return false;
        }

        /// <summary>
        /// Closes the running application with the same guid as the application that calls it.
        /// </summary>
        /// <param name="titleParts">An array of strings, all of which must be present in the title of the main window.</param>
        /// <returns>A bool indicating whether or not the instance was found and closed.</returns>
        public bool CloseRunningInstance(params string[] titleParts)
        {
            Process process;
            if (ProcessManager.TryGetProcess(out process, titleParts))
            {
                CloseProcess(process);
                return true;
            }

            return false;
        }

        #endregion

        #region PrivateMethods

        private void CreateMutex()
        {
            MutexAccessRule allowEveryoneRule = new MutexAccessRule(new SecurityIdentifier(WellKnownSidType.WorldSid, null), MutexRights.FullControl, AccessControlType.Allow);
            MutexSecurity securitySettings = new MutexSecurity();
            securitySettings.AddAccessRule(allowEveryoneRule);

            bool createdNew;
            _mutex = new Mutex(false, _mutexId, out createdNew, securitySettings);
        }

        private void ShowProcess(Process process)
        {
            if (IsIconic(process.MainWindowHandle))
            {
                ShowWindow(process.MainWindowHandle, SW_RESTORE);
            }

            WindowManager.MoveWindowToCentreScreen(process);

            SetForegroundWindow(process.MainWindowHandle);
            WindowManager.FlashWindow(process.MainWindowHandle);
        }

        private void CloseProcess(Process process)
        {
            ProcessManager.CloseProcess(process);

            IsInstanceRunning(); //This will take ownership of the mutex...
        }

        #endregion
    }
}

I have the User32 class exactly the same but it has been moved into the CompanyName.Win32 namespace now.

This uses the GuidFactory class that is used just to create a hashed Guid from the supplied string and is exposed publiclly:

using System;

namespace CompanyName.Factories
{
    /// <summary>
    /// A class to generate a System.Guid from different means than supplied in the base case
    /// </summary>
    public class GuidFactory
    {
        #region Methods

        /// <summary>
        /// Generates a hashed guid from the supplied text.
        /// </summary>
        /// <param name="text">The text to generate the guid from.</param>
        public static Guid GenerateHashedGuid(string text)
        {
            using (System.Security.Cryptography.MD5 md5 = System.Security.Cryptography.MD5.Create())
            {
                return new Guid(md5.ComputeHash(System.Text.Encoding.Default.GetBytes(text)));
            }
        }

        #endregion
    }
}

The next three classes are the managers to provide internal functionality. The are all internal static.

AssemblyManager:

using System;
using System.Reflection;
using System.Runtime.InteropServices;

namespace CompanyName.Managers
{
    internal class AssemblyManager
    {
        #region Methods

        internal static Guid? GetAssemblyGuid(Assembly assembly)
        {
            object[] customAttribs = assembly.GetCustomAttributes(typeof(GuidAttribute), false);
            if (customAttribs.Length < 1)
            {
                return null;
            }

            Guid guid;
            if (!Guid.TryParse(((GuidAttribute)customAttribs.GetValue(0)).Value, out guid))
            {
                return null;
            }

            return guid;
        }

        #endregion
    }
}

ProcessManager:

using System;
using System.Diagnostics;
using System.Linq;
using System.Reflection;

namespace CompanyName.Managers
{
    internal class ProcessManager
    {
        #region Methods

        internal static bool TryGetProcess(Guid guid, out Process process)
        {
            return TryGetProcess(out process, p =>
            {
                try
                {
                    Assembly assembly = Assembly.LoadFrom(p.MainModule.FileName);

                    Guid? processGuid = AssemblyManager.GetAssemblyGuid(assembly);
                    return processGuid.HasValue && guid.Equals(processGuid.Value);
                }
                catch { } //Is likely to throw an exception so just ignore it

                return false;
            });
        }

        internal static bool TryGetProcess(out Process process, params string[] titleParts)
        {
            return TryGetProcess(out process, p =>
            {
                return titleParts.Any(titlePart => !p.MainWindowTitle.Contains(titlePart));
            });
        }

        internal static bool TryGetProcess(out Process process, Func<Process, bool> isValidProcess)
        {
            Process current = Process.GetCurrentProcess();
            foreach (Process p in Process.GetProcesses())
            {
                if (p.Id == current.Id)
                {
                    continue;
                }

                if (isValidProcess(p))
                {
                    process = p;
                    return true;
                }
            }

            process = null;
            return false;
        }

        internal static void CloseProcess(Process process)
        {
            if (process.MainWindowHandle != IntPtr.Zero)
            {
                process.CloseMainWindow();
            }
            else
            {
                process.Kill();
            }

            process.WaitForExit(5000);
            if (!process.HasExited)
            {
                process.Kill();
            }
        }

        #endregion
    }
}

WindowManager:

using System;
using System.Diagnostics;
using System.Drawing;
using System.Linq;
using System.Runtime.InteropServices;
using System.Windows.Forms;

namespace CompanyName.Managers
{
    using static Win32.User32;

    internal class WindowManager
    {
        #region Constants

        private const uint WINDOW_FLASH_COUNT = 6;
        private const uint WINDOW_FLASH_RATE = 70;

        #endregion

        #region Methods

        internal static void MoveWindowToCentreScreen(Process process)
        {
            RECT rect;
            if (!GetWindowRect(new HandleRef(process, process.MainWindowHandle), out rect))
            {
                return;
            }

            Rectangle windowRect = new Rectangle(rect.Left, rect.Top, rect.Right - rect.Left + 1, rect.Bottom - rect.Top + 1);

            Screen screen = Screen.AllScreens.Where(s => s.Bounds.IntersectsWith(windowRect))
                                             .Select(s => new { Screen = s, Intersection = Rectangle.Intersect(s.WorkingArea, windowRect) })
                                             .OrderByDescending(o => o.Intersection.Width * o.Intersection.Height)
                                             .Select(o => o.Screen)
                                             .FirstOrDefault() ?? Screen.PrimaryScreen;

            int width = rect.Right - rect.Left;
            int x = screen.WorkingArea.X + ((screen.WorkingArea.Width / 2) - (width / 2));
            int height = rect.Bottom - rect.Top;
            int y = screen.WorkingArea.Y + ((screen.WorkingArea.Height / 2) - (height / 2));

            SetWindowPos(process.MainWindowHandle, 0, x, y, width, height, SWP_NOZORDER | SWP_NOSIZE | SWP_SHOWWINDOW);
        }

        internal static bool FlashWindow(IntPtr handle)
        {
            FLASHWINFO fi = new FLASHWINFO();
            fi.cbSize = Convert.ToUInt32(Marshal.SizeOf(fi));
            fi.hwnd = handle;
            fi.dwFlags = FLASHW_ALL;
            fi.uCount = WINDOW_FLASH_COUNT;
            fi.dwTimeout = WINDOW_FLASH_RATE;

            return FlashWindowEx(ref fi);
        }

        #endregion
    }
}

I'd like a complete re-review of the code as it has undergone some significant changes since the previous one was posted, with the core functionality staying the same.

You would now call it like:

[STAThread]
static void Main()
{
    using (InstanceManager intanceManager = new InstanceManager())
    {
        if (intanceManager.IsInstanceRunning())
        {
            intanceManager.ShowRunningInstance();
            return;
        }

        Application.EnableVisualStyles();
        Application.SetCompatibleTextRenderingDefault(false);
        Application.Run(new Form1());
    }
}

or:

[STAThread]
static void Main()
{
    using (InstanceManager intanceManager = new InstanceManager())
    {
        if (intanceManager.IsInstanceRunning())
        {
            intanceManager.CloseRunningInstance();
        }

        Application.EnableVisualStyles();
        Application.SetCompatibleTextRenderingDefault(false);
        Application.Run(new Form1());
    }
}

One of the main goals of this question is to find out how object orientated this approach is. I know I have a lot of static classes/methods but they feel like they all feel like they should be. So I'd be grateful if anyone could comment on that aspect of the design.

\$\endgroup\$
  • \$\begingroup\$ About #region - have you (and your colleagues) read this? \$\endgroup\$ – Mathieu Guindon Mar 7 '17 at 14:51
  • \$\begingroup\$ @Mat'sMug I agree regions aren't ideal in code and probably shouldn't be there, however, we have to as per the standard. Also our classes are usually quite large, mainly a lot of legacy code that just needs a fix here and there and changing one small lines kills the app in a thousand other ways (I'm sure we've all been there). So refactoring is usually too much work and the benefit doesn't outweigh the result. Because of this regions help it to easier find where we need to be in the code than spending hours and hours refactoring. \$\endgroup\$ – TheLethalCoder Mar 7 '17 at 14:56
  • \$\begingroup\$ Essentially for us they're more of a necessity than a want. A small company that doesn't have the time/resources to spend refactoring so we chuck regions at everything... so much so that it is now normal to us and is a standard. \$\endgroup\$ – TheLethalCoder Mar 7 '17 at 14:57
5
\$\begingroup\$

Just a very brief answer on one aspect. Objects that implement IDisposable generally have one of two behaviours when Dispose is called multiple times:

  1. They do nothing, i.e. you can call Dispose as many times as you want without any problems; first time the object gets disposed, on any further calls the object knows it has been disposed so doesn't do anything.
  2. They throw an ObjectDisposedException when called the second time.

Your implementation will release the mutex the first time it's called; on subsequent calls to Dispose you'll get an ApplicationException because you no longer own the mutex, but you try to release it anyway. This exposes implementation detail which the calling code shouldn't have to know about.

So I'd recommend making it behave in one the ways I listed.

\$\endgroup\$
  • \$\begingroup\$ Good point I usually have objects I can set to null in dispose after disposing of them so I don't need to use a state variable, completely forgot I'd have to use a field to keep track here though! \$\endgroup\$ – TheLethalCoder Feb 23 '17 at 8:59
  • 1
    \$\begingroup\$ The official guidelines explicitly say that calling Dispose more than once on an object shouldn't throw an exception. #2 seems wrong to me. Upvoted for #1. \$\endgroup\$ – Mathieu Guindon Mar 7 '17 at 15:00

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.