MessageQueue Disposed More Than Once

Question

I've seen this error on other posts, but not for this exact situation.

I have two classes that do the same thing with a MessageQueue. Because of that, I abstracted the creation and disposal of the queue to a helper class. I'm getting this error, and I can't see how the queue can be disposed more than once.

Object 'messageQueue' can be disposed more than once in method 'MsmqHelper.DisposeQueue(MessageQueue)'

In one of the classes, this is how the queue is used:

private MessageQueue _messageQueue;

Then, in the constructor of the class:

this._messageQueue = MsmqHelper.InitializeQueue();

Not that it really matters, but for completeness, here is where the queue is used:

this._messageQueue.Send(workflowCreated);

And here are the Dispose methods:

public void Dispose()
{
    Dispose(true);
    GC.SuppressFinalize(this);
}

private void Dispose(bool disposing)
{
    if (disposing == false) { return; }

    MsmqHelper.DisposeQueue(this._messageQueue);
}

And this is the code in the helper class that actually calls Dispose():

public static void DisposeQueue(MessageQueue messageQueue)
{
    if (messageQueue != null)
    {
        messageQueue.Close();
        messageQueue.Dispose();
        messageQueue = null;
    }
}

Where is it possible for the queue to be disposed more than once in this situation?

** Edit **

I thought it would be nice to add my comments, in the conversation below, here. It's a good summary, along with the accepted answer:

I think I get it now. The messageQueue method parameter has nothing to do with the original (this._messageQueue) reference to the object. So checking messageQueue for null, and setting it to null, does no good. The caller could still pass in its variable (this._messageQueue) even after being disposed. Hence, being able to be disposed more than once.

By the way, even setting the caller's variable (this._messageQueue) to null, in the calling method, doesn't help. The problem exists solely in MsmqHelper.DisposeQueue(). So the answer is to pass by ref or simply don't call DisposeQueue() and do it all in the calling method.

** Edit 2 **

After trying this, I get the same error. I simply don't get it.

public static void DisposeQueue(ref MessageQueue messageQueue)
{
    if (messageQueue == null) { return; }

    messageQueue.Close();
    messageQueue.Dispose();
    messageQueue = null;
}

** Edit 3 -- Bug? **

I'm starting to think that this may be a bug. If I comment messageQueue.Dispose(), the error goes away. HOWEVER, I can call messageQueue.Close() and messageQueue.Dispose() together in the calling method. Go figure. I think I'm just going to make these same calls from the calling methods, or only call Close() or Dispose() instead of both.

Answer 1

Yes. This can dispose the object multiple times:

The value that this._messageQueue evaluates to does not change after invoking MsmqHelper.DisposeQueue(this._messageQueue).

Only the local parameter (named messageQueue) was assigned the value null in the DisposeQueue method. Thus the "null guard" fails to correctly guard subsequent times around. (This is because C#'s default behavior is Call-By-Value: please see the link to understand what this means in context of "passing the value of a reference to an object".)

Either take in ref or assign this._messageQueue = null in the caller.

Answer 2

Close frees all resources of the MessageQueue object. See the documentation here. The error is most likely generated in CA because it sees that the execution path of Close also calls Dispose.

From the documentation:

    public  void ReceiveMessage()
    {
        // Connect to the a on the local computer.
        MessageQueue myQueue = new MessageQueue(".\\myQueue");

        // Set the formatter to indicate body contains an Order.
        myQueue.Formatter = new XmlMessageFormatter(new Type[]
            {typeof(String)});

        try
        {
            // Receive and format the message. 
            Message myMessage1 = myQueue.Receive();
            Message myMessage2 = myQueue.Receive();
        }

        catch (MessageQueueException)
        {
            // Handle sources of any MessageQueueException.
        }

        // Catch other exceptions as necessary.

        finally
        {
            // Free resources.
            myQueue.Close();
        }

        return;
    }

Close apparently will release the resources but will allow the component to reacquire them if they haven't been collected yet. It might be more prudent to open the MessageQueue object, use it, and then close it within the same call rather than open it for a period of time and closing it later because connection caching removes the overhead of opening the MessageQueue in repeated calls.

*UPDATE* It appears that CA treats CA2202 differently for member fields versus passing the disposable object to a method, even if that method is private to the class. Regardless, according to the documentation, you should only have to call Close() or Dispose() but not both. I recommend however changing your design so that you create, use, and then close the MessageQueue object all within the scope of your message operations like the example from the documentation example demonstrates above.

Answer 3

If MessageQueue class implements IDisposable iterface, then there is no point to use Dispose method explicitly and Close() method, because in all such classes Close() method is usually is not an iterface method but rather a class method. Typically, in Dispose method all correct impementation should call Close() mehod before release managed/unmanaged resources.

Again, by impelmenting external static helper, you break the Disposable pattern. It's not the correct way to control lifetime of the object; You don't need to mess up with Disposable pattern, you can simply use it

And your code might be simplified like this:

    // 1. Use static class. By the agreement, all helper classes should be static to avoid 
    // IDisposable inheritance, in example
    public static class MsmqHelper//: IDisposable
    {
        //private MessageQueue _messageQueue;

        //public MessageQueueHelper(bool workflowCreated)
        //{
        //    this._messageQueue = MsmqHelper.InitializeQueue();
        //    this._messageQueue.Send(workflowCreated);
        //}

        public static SendMessage(object workflowCreated)
        {
            // 2. If static method in static class does not takes parameters, 
            // I might be better to to implicitly call the constructor?

            // using(MessageQueue msmsq = MsmqHelper.InitializeQueue())

            using(MessageQueue msmsq = new MessageQueue())
            {
                msmq.Send(workflowCreated);
                msmq.Close(); 

                // MsmqHelper.DisposeQueue(msmq);

                // 3. You should explicitly call Close object to immediately release     
                // unmanaged resources, while managed ones will be released 
                // at next GC rounds, as soon as possible
            }
        }
        //private MessageQueue _messageQueue;

        //public void Dispose()
        //{
        //    Dispose(true);
        //    GC.SuppressFinalize(this);
        //}

        //private void Dispose(bool disposing)
        //{
    //    if (disposing == false) { return; }
    //
    //    MsmqHelper.DisposeQueue(this._messageQueue);
    //}

    //public static void DisposeQueue(MessageQueue messageQueue)
    //{
    //    if (messageQueue != null)
    //    {
    //        messageQueue.Close();
    //        messageQueue.Dispose();
    //        messageQueue = null;
    //    }
    //}
}