Ignore cancelled tasks in exception handling middleware ASP.NET WebAPI

Question

I want to avoid logging different types of cancelled requests, and I'm not sure what is the best practice here. I found in GitHub post wehre the developer is checking for different types this way:

private static bool IsCancellationException(Exception ex)
{
    if (ex == null) return false;
    if (ex is OperationCanceledException)
        return true;
    if (ex is System.IO.IOException && ex.Message == "The client reset the request stream.")
        return true;
    if (ex is Microsoft.AspNetCore.Connections.ConnectionResetException)
        return true;
    // .NET SQL has a number of different exceptions thrown when operations are cancelled
    if (ex.Source == "Microsoft.Data.SqlClient" && ex.Message == "Operation cancelled by user.")
        return true;
    if ((ex is Microsoft.Data.SqlClient.SqlException || ex is System.Data.SqlClient.SqlException) &&
        ex.Message.Contains("Operation cancelled by user"))
        return true;
    return false;
}

But it seems really ugly to me so I came up with this:

// ExceptionHandlingMiddleware
public async Task InvokeAsync(HttpContext context)
{
    try
    {
        await _next(context);
    }
    catch (Exception ex) when (context.RequestAborted.IsCancellationRequested)
    {
        HandleCancellationException(context, ex);
    }
    catch (Exception ex)
    {
        await HandleExceptionAsync(context, ex);
    }
}

Wouldn't checking context.RequestAborted.IsCancellationRequested be enough to ignore cancelled tasks?

From the docs I read that IsCancellationRequested only guarantees that cancellation was requested, so I guess it's possible that the task wasn't really cancelled and failed, but then would I really care since the client doesn't either?

This obviously doesn't handle the case where the task was cancelled by the server, but I actually want to see those in the logs, but not client initiated cancellations.

I could also handle OperationCanceledException but I think that's already covered by the first catch, so I probably don't need it and anyway I'm only interested in ignoring client cancellations.

What is the best practice here? Thank you!

BTW Using .NET 6

UPDATE:

I checked the default ExceptionHandlerMiddleware in ASP.NET Core and this is what they do:

if ((edi.SourceException is OperationCanceledException || edi.SourceException is IOException)
    && context.RequestAborted.IsCancellationRequested)
{
    _logger.RequestAbortedException();

    if (!context.Response.HasStarted)
    {
        context.Response.StatusCode = StatusCodes.Status499ClientClosedRequest;
    }

    return;
}

I could do the same, but would need to handle more types such as SqlException, and in some cases I need to check ex.GetBaseException() is OperationCanceledException because I noticed that for example CsvWriter library wraps the OperationCanceledException in a custom exception. I still think it's probably fine to catch Exception only, but not sure...

UPDATE 2:

I checked production logs, and interestingly I found TaskCanceledException entries where IsCancellationRequested was false, and I even managed to reproduce such a cancellation myself! It seems like it's not necessarily always even true even if the client cancelled the request...

It was really hard to reproduce though, but I got it to happen once. I noticed one differnce between these similar exceptions (except for the request being cancelled) was the existence of an AsyncState property (I'm writing all exception data as JSON). In any case, so far I've gone with this approach:

public async Task InvokeAsync(HttpContext context)
{
    try
    {
        await _next(context);
    }
    catch (OperationCanceledException oce)
    {
        HandleCancellationException(context, oce);
    }
    catch (Exception ex) when (context.RequestAborted.IsCancellationRequested)
    {
        // Ignore any cancellation-related exceptions by checking if request was cancelled
        HandleCancellationException(context, ex);
    }
    catch (Exception ex)
    {
        await HandleExceptionAsync(context, ex);
    }
}

Perhaps it's better to ask the ASP.NET team on GitHub about this...

They "should" be equal so checking of one might be sufficient. But we are talking about a modular system were the relevant part might have been exchanged and, reading stuff here, there are always bugs like the one mentioned [here](https://stackoverflow.com/questions/47153525/whats-the-difference-between-httpcontext-requestaborted-and-cancellationtoken-p). To be on the safe side check for both. — Ralf, Apr 12 '23 at 09:10
If I catch `OperationCanceledException` then tasks canceled by the server (if such can happen) would not be logged. I still don't see how adding that would help, unless I misunderstood your point? — Shahin Dohan, Apr 12 '23 at 18:14

Ignore cancelled tasks in exception handling middleware ASP.NET WebAPI

0 Answers0