Am I obliged to call EndPeek after using BeginPeek?

Question

I have a Windows service that processes a private, local message queue (MSMQ). When it starts, it registers an event handler for PeekCompleted on the queue, and it then calls the asynchronous BeginPeek() to wait for a message to arrive.

protected override void OnStart(string[] args)
{
    if (String.IsNullOrWhiteSpace(args[0]))
        return;

    queue = new MessageQueue(args[0]);
    queue.Formatter = new BinaryMessageFormatter();
    queue.PeekCompleted += new PeekCompletedEventHandler(OnPeekCompleted);

    queue.BeginPeek();
}

Once a message arrives, my goal is to obviously process that message. My code currently has a queue.Receive() method to get the message, contained within a transaction so the message gets put back on the queue in case of errors during processing. BeginPeek() is called again to restart the cycle.

private static void OnPeekCompleted(Object source, PeekCompletedEventArgs asyncResult)
{
    try
    {
        MessageQueue q = (MessageQueue)source;

        using (MessageQueueTransaction trans = new MessageQueueTransaction())
        {
            trans.Begin();

            Message msg = q.Receive(trans);
            ProcessMessage(msg);

            trans.Commit();
         }

         // Restart the asynchronous peek operation.
         q.BeginPeek();
    }
    catch (MessageQueueException qEx)
    {
        // TODO: Walk it off.
    }
    return;
}

Do I, at any point, need to call EndPeek() on the queue ?

Maybe to avoid memory leaks, like this question alludes to ? I'm pretty sure I don't have to, but the documentation isn't very clear on that. It just doesn't feel 100% right to 'begin' something without 'ending' it :)

Btw: I could replace the Receive() line with Message msg = q.EndPeek(asyncResult.AsyncResult), which equally fetches me the message, but it doesn't remove the message from the queue.

Answer 1

Giving a proper answer to this question takes some effort, because the short answer ("no") could be misleading.

Does the API description explicitly say you must call EndPeek() for every call to BeginPeek()? Not in any topic I could find, and not only that, it appears to state the opposite here:

To use BeginPeek, create an event handler that processes the results of the asynchronous operation, and associate it with your event delegate. BeginPeek initiates an asynchronous peek operation; the MessageQueue is notified, through the raising of the PeekCompleted event, when a message arrives in the queue. The MessageQueue can then access the message by calling EndPeek(IAsyncResult) or by retrieving the result using the PeekCompletedEventArgs.

(Emphasis mine.) This seems to say that you can either use .EndPeek() or just directly get the message from the event args with no obligation to call .EndPeek().

Alright, so does the implementation mandate that you call .EndPeek() in order to make things work correctly? At least for the System.Messaging implementation in .NET 4.0, the answer is no. When you call .BeginPeek(), an asynchronous operation is allocated and a callback is registered for completion. The unmanaged resources associated with this operation are partially cleaned up in this callback, and only then is the event handler called. .EndPeek() does not actually do any cleanup -- it merely waits for the operation to complete if it hasn't yet, checks for errors, and returns the message. So it is indeed true that you can either call .EndPeek() or just access the message from the event args, or do nothing at all -- it will all work just as poorly.

Poorly, yes -- note that I said "partially cleaned up". The implementation of MessageQueue has a problem in that it allocates a ManualResetEvent for every asynchronous operation, but never disposes it, leaving this entirely up to the garbage collector -- something .NET developers are often excoriated for doing, but of course Microsoft's own developers aren't perfect either. I haven't tested whether the OverlappedData leak described in this question is still relevant, nor is it immediately obvious from the source, but it would not surprise me.

The API has other warning signs that its implementation may leave something to be desired, most prominently that it does not follow the established .Begin...() / .End...() pattern for asynchronous operations but introduces event handlers in the middle, producing a strange hybrid I've never see anywhere else. Then there's the very dubious decision of making the Message class inherit from Component, which adds considerable overhead to every instance and raises the question of when and how it should be disposed... all in all, not Microsoft's best work.

Now, does all this mean you're not "obliged" to call .EndPeek()? Yes, in the sense that calling it or not calling it makes no functional difference with regards to resource cleanup or correctness. But with all that said, my advice is still to call it anyway. Why? Because anyone who is familiar with how the asynchronous operation pattern works in other .NET classes would expect the call to be there, and not putting it there looks like a bug that could lead to resource leaks. If there is a problem with the application, such a person might reasonably spend some fruitless effort looking at "problem code" that isn't. Given that a call to .EndPeek() has negligible overhead compared to the rest of the machinery, I'd say the savings in programmer surprise more than make up for the costs. A possible alternative is to insert a comment instead, explaining why you're not calling .EndPeek() -- but in all probability this still takes more programmer cycles to grasp than just calling it.

In theory, another reason for calling it is that the semantics of the API could change in the future to make the call to .EndPeek() necessary; in practice, this is very unlikely to happen because Microsoft is traditionally reluctant to make breaking changes like this (code that previously and reasonably did not call .EndPeek() would stop working) and the existing implementation already contravenes established practice.