4
\$\begingroup\$

This code works, as far as I can tell, but there's something that just feels wrong about it. The intended flow is like this:

  • If no dialog is showing
    • Show dialog, return result
  • else
    • Move dialog into view, return no result

It sounds easy enough, but because there's a possiblity of multiple threads I have to use locking. However, because the ShowDialog() call is blocking, I couldn't just throw a lock around the whole thing, so instead my code is littered with Monitor.Enter and Monitor.Exit calls. It gets even more complicated due to exception handling.

interface IDialogService
{
    bool? ShowAuthenticationDialog();
}

class DialogService : IDialogService
{
    private readonly IServiceProvider _serviceProvider;

    public DialogService(IServiceProvider serviceProvider)
    {
        _serviceProvider = serviceProvider;
    }

    private readonly object _authenticationDialogLock = new object();
    private bool _isAuthenticationDialogShowing;
    private AuthenticationDialog _authenticationDialog;

    public bool? ShowAuthenticationDialog()
    {
        Monitor.Enter(_authenticationDialogLock);
        if (!_isAuthenticationDialogShowing)
        {
            try
            {
                _isAuthenticationDialogShowing = true;
                _authenticationDialog = _serviceProvider.GetService<AuthenticationDialog>();
                Monitor.Exit(_authenticationDialogLock);

                var authenticationDialogResult = _authenticationDialog.ShowDialog();

                Monitor.Enter(_authenticationDialogLock);
                return authenticationDialogResult;
            }
            catch
            {
                return null;
            }
            finally
            {
                _authenticationDialog = null;
                _isAuthenticationDialogShowing = false;

                // Need this check because an exception could come from ShowDialog()
                // where we're not locked.
                if (Monitor.IsEntered(_authenticationDialogLock))
                    Monitor.Exit(_authenticationDialogLock);
            }
        }

        _authenticationDialog.Activate();
        Monitor.Exit(_authenticationDialogLock);
        return null;
    }
}

Can I make this less "messy" somehow? Have I missed any obvious cases where the constant grab-and-release of the lock can lead to problems?

I feel like there could be an edge case where a thread can call Activate() before ShowDialog() has been called or after it has returned, but I honestly can't think of how to protect against that specific case because ShowDialog() is blocking and thus has to release its lock.

Oh, and if anyone's wondering, the GetService<>() method on the IServiceProvider is an extension method that looks like this:

public static class ServiceProviderExtensions
{
    public static T GetService<T>(this IServiceProvider serviceProvider)
    {
        return (T)serviceProvider.GetService(typeof(T));
    }
}
\$\endgroup\$
1
\$\begingroup\$

Shout in the dark, really...

public class DialogService : IDialogService
{
    private static readonly ConcurrentQueue<AuthenticationDialog> DialogQueue = 
        new ConcurrentQueue<AuthenticationDialog>();
    private static volatile bool _isShowing;

    public DialogService(IServiceProvider serviceProvider)
    {
        DialogQueue.Enqueue(serviceProvider.GetService<AuthenticationDialog>());
    }

    public bool? ShowAuthenticationDialog()
    {
        AuthenticationDialog dialog;
        DialogQueue.TryDequeue(out dialog);

        if (dialog == null)
            return null;

        if (_isShowing)
        {
            dialog.Activate();
            return null;
        }

        _isShowing = true;
        bool? result = null;

        try
        {
            result = dialog.ShowDialog();
        }
        catch 
        {}
        finally
        {
            _isShowing = false;
        }

        return result;
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ Not a bad idea, using a concurrent collection! :) Your code as it is written won't work too well though, but I understand what you were going for, and I can probably make use of this. Some issues: On the first call, the TryDequeue method will remove the AuthenticationDialog, and all the others will get null, and just return. I.e., they won't get to call Activate(), one of my requirements. Also, this will only ever work once, because you never add another AuthenticationDialog back to the queue. \$\endgroup\$ – Alex Feb 22 '15 at 7:59
  • \$\begingroup\$ I have doubts it's working 100%, too, but glad if it pointed you in right direction. I did, however, test by firing off multiple delayed Task.Run()s using same- and new instances of dialog service. It did fire Activate() multiple times while messagebox was visible (wrapped default MessageBox as AuthenticationDialog). So static ConcurrentQueue was filled (seemingly) correct. \$\endgroup\$ – Mikko Viitala Feb 22 '15 at 17:21

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.