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?

When Microsoft explicitly adds a method that throws an exception then, no, "bad practice" is not the first thing that jumps to mind. Only pretending that it did not get thrown is a bad practice. — Hans Passant, May 31 '16 at 10:41
I wouldn't even put `ct.ThrowIfCancellationRequested()` at the end of `ConnectAsync`. What if the token gets cancelled right after this call but before `ConnectAsync` returns? I'd rather let the client of the library deal with various racing exceptions and would just document possible side effect. — noseratio, May 31 '16 at 23:42
@Noseratio That `ct.ThrowIfCancellationRequested()` is there because without it, there's a subtle race where `using (ct.Register(...))` can close the opened connection even as our `ConnectAsync()` continues on to success. It might not make a difference in our example (and the caller will probably dispose the connection anyhow), but that structure is probably worth kept in place as a rule of thumb, to preserve integrity (atomicity) in cases where our methods go on to change states. — antak, Jun 01 '16 at 00:18
why you are calling ct.ThrowIfCancellationRequested(); after await. what purpose does it solve? — Saravanan, Jun 01 '16 at 00:24
@Saravanan Do you mean in the `ConnectSslStreamAsync()` example? Isn't after `await`, before more work, one of the most standard places to put them? (Because awaits resume after an asynchronous intermission, and much of the world may have changed in the interim.) — antak, Jun 01 '16 at 01:52
You can stick to general practice by throwing proper exception upon cancellation. If I have understood your problem correctly, [this](http://stackoverflow.com/a/23618864/1661861) would help. — Saravanan, Jun 01 '16 at 03:28
@Saravanan *You can stick to general practice by throwing proper exception upon cancellation.*: I posted the question because I don't know what's general practice or proper. If you know this for a fact, I think it's better if you could post this as an answer. — antak, Jun 01 '16 at 04:24

score 2 · Answer 1 · edited May 23 '17 at 12:33

2

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.

edited May 23 '17 at 12:33

Community

1
1

answered Jun 01 '16 at 05:01

Saravanan

920
9
22

I'm not sure what you mean by your second paragraph. The answer you linked to talks about changing an `OperationCancelledException` to a `TimeoutException` when one uses a second `CancellationTokenSource` for the purpose of timing out, and that's possible by comparing the exception's `CancellationToken` field (as shown in the accepted answer). The question here is about changing some exception X into an `OperationCancelledException`, but that doesn't seem to justify using `.WithCancellation()` when one can just `catch (Exception) { ct.ThrowIfCancellationRequested; throw; }` as shown above. – antak Jun 01 '16 at 05:22

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

1 Answers1