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.

Is there any compelling *reason* for not calling `.EndPeek()`? If you don't need the result of `.EndPeek()`, you're free to discard it, but not calling it at all is almost certainly a bad idea -- you are essentially banking on `.BeginPeek()` not allocating any unmanaged resources that `.EndPeek()` has to release. Even if that's true now, who says the next release won't break that assumption? — Jeroen Mostert, Nov 18 '14 at 15:24
Side note: pleinolijf, your post reads like "my code looks incorrect, any chance it will work reliably". Personally I'd not do that irrespective of possibility of code working correctly - time wasted on finding/fixing missing EndXXXX call by all future readers of the code would likely out-weight any "savings" you will make. — Alexei Levenkov, Nov 18 '14 at 15:24
Fair points, the both of you. And that's essentially what I'm asking: is it safe to not call EndPeek ? Does BeginPeek indeed allocate any resources ? That's what's not clear to me, although obviously, the naming of the methods do suggest that. What I'm looking for, is facts, not assumptions. — pleinolijf, Nov 18 '14 at 15:33
To add to @JeroenMostert's comment: see my last paragraph. The message, it seems, remains in the queue. Seems illogical to me to call `EndPeek()` first, and then `Receive()` for good measure... — pleinolijf, Nov 18 '14 at 15:36
It's not clear what fact you are after. Do you want the designer of the API to explain to you what precisely they meant? Certainly, learning that whatever version of `System.Messaging.dll` you're using has a `.BeginPeek()` implementation that does or does not allocate resources would prove exactly nothing, other than that your code *currently* does (not) work. — Jeroen Mostert, Nov 18 '14 at 15:40
Yes, it is redundant to call `EndPeek()` and then `Receive()`. But that's because you decided to call `BeginPeek()` rather than `BeginReceive()`. Would you consider it logical for `Receive()` to have to check for any outstanding `BeginPeek()` requests to clean them up? — Jeroen Mostert, Nov 18 '14 at 15:42
I want to use `BeginPeek()`, so the actual receiving of the message can be done in the scope of a transaction. I didn't manage to do that with `BeginReceive()` in this kind of setup. Is there a way to use `EndPeek()` and next to remove the message from the queue (without using `Receive()`? — pleinolijf, Nov 18 '14 at 15:52
Not that I can see. Also, the MSDN explicitly says you cannot use `BeginReceive()` with transactions, but must instead use `BeginPeek()`, so that's OK -- it does not then go on to say that you are free not to call `EndPeek()`, though. I still don't see the problem -- just call `EndPeek(asyncResult)` while ignoring the result and be done with it. This is likely to not even involve any (avoidable) overhead, since you already started the peek operation anyway -- not completing it doesn't make that go away. — Jeroen Mostert, Nov 18 '14 at 15:59
@JeroenMostert yeah, I was thinking of going that route to be on the safe side. It just doesn't look very nice. If you put your last comment as an answer, I will accept it. — pleinolijf, Nov 19 '14 at 07:04
I'd love to, but I can't, because if you put me on the line like that I have to actually dig into it to find out I was wrong. Sorry about that -- I hope the answer makes up for it. :-) — Jeroen Mostert, Nov 19 '14 at 08:13
Is there a guarantee that the returned message of q.Receive() is the same than the one that would have been returned by q.EndPeek(e.asyncResult)? On a multiple listener setup, I wonder if it would lead to losing messages, or messages being left in the original queue. — Micaël Félix, Sep 22 '16 at 14:25

score 11 · Accepted Answer · edited May 23 '17 at 12:01

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.

Am I obliged to call EndPeek after using BeginPeek?

1 Answers1