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

Question

Is it bad practice for my libraries to exit methods throwing something other than OperationCancelledException when a CancellationToken.IsCancelRequested detected?

For instance:

 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; } 

When invalidated, it has the ability to throw an ObjectDisposedException or NullReferenceException depending on the time. (Because TcpClient.ConnectAsync() can throw one when TcpClient.Close() is called at the same time.)

Now I could fix it like this:

 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 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 also at each level 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 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 this extra code, when in fact all that is required is a single check on the outermost layer. (In the source of the cancellation.)

Does bad practice allow these arbitrary exceptions to fly? Or ignore them at the most external caller?

Answer 1

You must throw an OperationCancelledException if invalidation is required. When the consumers of your code receive an exception, they have no idea whether it is a cancellation or something else.

However, if you cannot throw an OperationCancelledException due to registering a private delegate, you can try this one here , where you will create a task to close the tcpClient or thread and verify that the first task has completed and take appropriate action.