Is it bad practice to throw an arbitrary exception when CancellationToken is set?

Question

Is it bad practice to have my libraries break out of methods by throwing something other than an OperationCancelledException when CancellationToken.IsCancelRequested is detected?

For example:

async Task<TcpClient> ConnectAsync(string host, int port, CancellationToken ct)
{
    var client = new TcpClient();
    try
    {
        using (ct.Register(client.Close, true))
        {
            await client.ConnectAsync(host, port);
        }

        // Pick up strugglers here because ct.Register() may have hosed our client
        ct.ThrowIfCancellationRequested();
    }
    catch (Exception)
    {
        client.Close();
        throw;
    }

    return client;
}

Upon cancellation, this has the possibility of throwing ObjectDisposedException or NullReferenceException depending on the timing. (Because TcpClient.ConnectAsync() can throw either one when TcpClient.Close() is called concurrently.)

Now, I could fix this like so:

async Task<TcpClient> ConnectAsync(string host, int port, CancellationToken ct)
{
    var client = new TcpClient();
    try
    {
        using (ct.Register(client.Close, true))
        {
            try
            {
                await client.ConnectAsync(host, port);
            }
            catch (Exception)
            {
                // These exceptions are likely because we closed the
                // connection with ct.Register().  Convert them to
                // OperationCancelledException if that's the case
                ct.ThrowIfCancellationRequested();
                throw;
            }
        }

        // Pick up strugglers here because ct.Register() may have hosed our client
        ct.ThrowIfCancellationRequested();
    }
    catch (Exception)
    {
        client.Close();
        throw;
    }

    return client;
}

And likewise at every layer of the call hierarchy where applicable:

async Task<TcpClient> ConnectSslStreamAsync(string host, int port, CancellationToken ct)
{
    var client = await ConnectAsync(host, port, ct);
    try
    {
        ct.ThrowIfCancellationRequested();

        var sslStream = new SslStream(client.getStream());
        using (ct.Register(sslStream.Close))
        {
            try
            {
                await sslStream.AuthenticateAsClientAsync(...);
            }
            catch (Exception)
            {
                // These exceptions are likely because we closed the
                // stream with ct.Register().  Convert them to
                // OperationCancelledException if that's the case
                ct.ThrowIfCancellationRequested();
                throw;
            }
        }

        // Pick up strugglers here because ct.Register() may have hosed our stream
        ct.ThrowIfCancellationRequested();
    }
    catch (Exception)
    {
        client.Close();
        throw;
    }

    return client;
}

But this adds these extra code when, in reality, all that's required is one check at the very outer layer. (At the cancellation source.)

Is it bad practice to let these arbitrary exceptions fly? Or are ignoring them at the outermost caller conventional?

Answer 1

You should throw OperationCancelledException if cancellation is requested. When the consumers of your code gets exception, they will have no clue if it was actually a cancellation or something else.

That said, if you are not able to throw OperationCancelledException because of registering close delegate, you could try the approach provided here where you will create a task for closing the tcpClient or stream and verify which task has completed first and take action accordingly.