Is it safe to call BeginXXX without calling EndXXX

问题:

When using the Asynchronous Programming Model it is usually recommended to match every BeginXXX with an EndXXX, otherwise you risk leaking resources until the asynchronous operation completes.

Is that still the case if the class implements IDisposable and the instance was disposed by calling Dispose?

If for example I use UdpClient.BeginReceive in a UdpListener:

class UdpListener : IDisposable
{
    private bool _isDisposed;
    private readonly IPAddress _hostIpAddress;
    private readonly int _port;
    private UdpClient _udpClient;
    public UdpListener(IPAddress hostIpAddress, int port)
    {
        _hostIpAddress = hostIpAddress;
        _port = port;
    }
    public void Start()
    {
        _udpClient.Connect(_hostIpAddress, _port);
        _udpClient.BeginReceive(HandleMessage, null);
    }
    public void Dispose()
    {
        if (_isDisposed)
        {
            throw new ObjectDisposedException("UdpListener");
        }
        ((IDisposable) _udpClient).Dispose();
        _isDisposed = true;
    }
    private static void HandleMessage(IAsyncResult asyncResult)
    {
        // handle...
    }
}

Do I still need to make sure UdpClient.EndReceive is called on the disposed _udpClient (which will just result in an ObjectDisposedException)?

---

Edit:

It isn't uncommon to dispose a UdpClient (and other IDisposables) before all asynchronous operations completed as a way to cancel or implement a timeout, especially over operations that will never complete. This is also what's recommended throughout this site.

回答1:

When using the Asynchronous Programming Model it is usually recommended to match every BeginXXX with an EndXXX, otherwise you risk leaking resources kept while the asynchronous operation is still "running".

Is that still the case if the class implements IDisposable and Dispose was called on the instance?

This has nothing to do with a class implementing IDisposable or not.

Unless you can be sure that the async completion will free up any resources tied up with the async operation initiated through BeginXXX, and no cleanup is performed in, or as a result of the EndXXX call, you need to ensure that you match your calls. The only way to be certain of this, is to inspect the implementation of a specific async operation.

For the UdpClient example you chose, it happens to be the case that:

1. Calling EndXXX after disposing the UDPClient instance will result in it directly throwing an ObjectDisposedException.
2. No resources are disposed in or as a result of the EndXXX call.
3. The resources tied up with this operation (native overlapped and pinned unmanaged buffers), will be recycled on the async operation completion callback.

So in this case it is perfectly safe, without leakage.

As a general approach

I don't believe this approach is correct as a general approach, because:

1. The implementation could change in the future, breaking your assumptions.
2. There are better ways to do this, using cancellation and time-outs for your async (I/O) operations (e.g. by calling Close on the _udpClient instance to force an I/O failure).

Also, I would not want to rely on me inspecting the entire call stack (and not making a mistake in doing so) to ensure that no resources will be leaked.

Recommended and documented approach

Please note the following from the documentation for the UdpClient.BeginReceive method:

The asynchronous BeginReceive operation must be completed by calling the EndReceive method. Typically, the method is invoked by the requestCallback delegate.

And the following for the underlying Socket.BeginReceive method:

The asynchronous BeginReceive operation must be completed by calling the EndReceive method. Typically, the method is invoked by the callback delegate.

To cancel a pending BeginReceive, call the Close method.

I.e. this is the "by design" documented behavior. You can argue whether the design is very good, but it is clear in what the expected approach to cancellation is, and the behavior that you can expect as the result of doing so.

For your specific example (updated to do something useful with the async result), and other situations similar to it, the following would be an implementation that follows the recommended approach:

public class UdpListener : IDisposable
{
    private readonly IPAddress _hostIpAddress;
    private readonly int _port;
    private readonly Action<UdpReceiveResult> _processor;
    private TaskCompletionSource<bool> _tcs = new TaskCompletionSource<bool>();
    private CancellationTokenSource _tokenSource = new CancellationTokenSource();
    private CancellationTokenRegistration _tokenReg;
    private UdpClient _udpClient;

    public UdpListener(IPAddress hostIpAddress, int port, Action<UdpReceiveResult> processor)
    {
        _hostIpAddress = hostIpAddress;
        _port = port;
        _processor = processor;
    }

    public Task ReceiveAsync()
    {
        // note: there is a race condition here in case of concurrent calls 
        if (_tokenSource != null && _udpClient == null)
        {
            try 
            {
                _udpClient = new UdpClient();
                _udpClient.Connect(_hostIpAddress, _port);
                _tokenReg = _tokenSource.Token.Register(() => _udpClient.Close());
                BeginReceive();
            }
            catch (Exception ex)
            {
                _tcs.SetException(ex);
                throw;
            }
        }
        return _tcs.Task;
    }

    public void Stop()
    {
        var cts = Interlocked.Exchange(ref _tokenSource, null);
        if (cts != null)
        {
            cts.Cancel();
            if (_tcs != null && _udpClient != null)
                _tcs.Task.Wait();
            _tokenReg.Dispose();
            cts.Dispose();
        }
    }

    public void Dispose()
    {
        Stop();
        if (_udpClient != null) 
        {
            ((IDisposable)_udpClient).Dispose();
            _udpClient = null;
        }
        GC.SuppressFinalize(this);
    }

    private void BeginReceive()
    {
        var iar = _udpClient.BeginReceive(HandleMessage, null);
        if (iar.CompletedSynchronously)
            HandleMessage(iar); // if "always" completed sync => stack overflow
    }

    private void HandleMessage(IAsyncResult iar)
    {
        try
        {
            IPEndPoint remoteEP = null;
            Byte[] buffer = _udpClient.EndReceive(iar, ref remoteEP);
            _processor(new UdpReceiveResult(buffer, remoteEP));
            BeginReceive(); // do the next one
        }
        catch (ObjectDisposedException)
        {
            // we were canceled, i.e. completed normally
            _tcs.SetResult(true);
        }
        catch (Exception ex)
        {
            // we failed.
            _tcs.TrySetException(ex); 
        }
    }
}

回答2:

Considering the facts that Dispose (which should be identical to Close¹) releases any unmanaged resources (the GC releases the managed ones) and methods throw ObjectDisposedException when called on a disposed instance² it should be safe to not call EndXXX.

That behavior of course depends on the specific implementation but it should be safe and it is indeed the case in UdpClient, TcpClient, Socket and more...

Since APM predates the TPL and the CancelationToken that came with it you usually can't use CancelationToken to cancel these asynchronous operations. That's why you also can't pass a CancelationToken on the equivalent async-await methods (e.g. UdpClient.RecieveAsync) as they are just a wrapper over the BeginXXX/EndXXX methods with a call to Task.Factory.FromAsync. Moreover timeouts (like Socket.ReceiveTimeout) usually only affect the synchronous options and not the asynchronous ones.

The only way to cancel this type of operations is by disposing the instance itself³ which releases all resources and invokes all the waiting callbacks which in turn usually call EndXXX and get the appropriate ObjectDisposedException. This exception is usually raised from the very first line of these methods when the instance is disposed.

From what we know about APM and IDisposable calling Dispose should be enough to clear out any hanging resources and adding a call to EndXXX will just raise an unhelpful ObjectDisposedException and nothing more. Calling EndXXX may protect you where the developer didn't follow the guidelines (it may not, it depends on the faulty implementation) but not calling it would be safe in many if not all of .Net's implementations and should be safe in the rest.

---

1. "Consider providing method Close(), in addition to the Dispose(), if close is standard terminology in the area. When doing so, it is important that you make the Close implementation identical to Dispose and consider implementing the IDisposable.Dispose method explicitly"

2. "Do throw an ObjectDisposedException from any member that cannot be used after the object has been disposed of".

3. "To cancel a pending call to the BeginConnect method, close the Socket. When the Close method is called while an asynchronous operation is in progress, the callback provided to the BeginConnect method is called."