Thread safety when using IExceptionHandlerFeature?

[aymericb]

I have code that use ASP.NET Core to implement a JSON API. In order to deal with errors it does something similar to this:

app.UseExceptionHandler(action =>
{
    action.Run(async context =>
    {
        // Apply CORS to error response - workaround for https://github.com/aspnet/CORS/issues/90
        var policy = await app.ApplicationServices.GetService<ICorsPolicyProvider>().GetPolicyAsync(context, ALLOW_ALL);
        var result = cors.EvaluatePolicy(context, policy);
        cors.ApplyResult(result, context.Response);

        // Get exception and log it
        var exception = context.Features.Get<IExceptionHandlerFeature>()?.Error;
        await LogException(exception, context.Request);

        // Return API JSON error to the client
        var error = (exception as ApiException)?.Error ?? Models.Error.Unknown;
        await WriteErrorResponse(context, error);
    });
});

I noticed recently that under some circumstances the exception objects that were logged were identical. This occurred when the server was under load and similar exceptions were generated a few ms apart, so it made me think it was a concurrency issue.

After some digging, I believe this problem occurs because access to the IFeatureCollection is not thread safe (see ApplicationInsights-aspnetcore #373), and because there is an await before retrieving the exception from IFeatureCollection.

My understanding of concurrency bug is as follow:

• The API throw an exception in on of the controllers
• The ExceptionHandlerMiddleware used by UseExceptionHandler() catches the exception and sets it on the context (context.Features.Set<IExceptionHandlerFeature>(exceptionHandlerFeature))
• Because of that CORS workaround, there is an await executed next. I believe this means the code that follows could potentially be executed in a different thread.
• In the next line I access the FeatureCollection via context.Features.Get<IExceptionHandlerFeature>()?.Error
• Because the IFeatureCollection is not thread safe, and it reads the IExceptionHandlerFeature in a different thread from where it was written, I end up with corrupted data (i.e. the exception object from another HttpContext)

Does that make sense? Or am I just missing something obvious here? Unfortunately, I don't have definite proof. I was not able to reproduce the problem on a test server.

But if I am right, the whole IExceptionHandlerFeature being set inside the ExceptionHandlerMiddleware is kind of misleading. It would only safe to set features early on when the HttpContext is being initialized, and not in the way it is done for IExceptionHandlerFeature. It looks like the ExceptionHandlerMiddleware should be modified. Maybe it would be better if the Exception object was given as a parameter of the ExceptionHandler callback?

I decided to write my own ErrorMiddleware which is similar to ExceptionHandlerMiddleware but avoids using a Dictionary to capture the exception. I suppose I could have simply reordered the code and ensured the context.Features.Get<IExceptionHandlerFeature>() was executed immediately, and not after the CORS workaround.

[Tratcher]

I don't follow, await's are thread safe. You should only get corruption if you're accessing the same objects from different threads at the same time. The await stops operations on the first thread and transfers them to a new one, there's no concurrent execution.

[aymericb]

The await stops operations on the first thread and transfers them to a new one, there's no concurrent execution.

Yes. I understand there is no concurrent execution. However I was thinking the issue is that a memory barrier would be needed before reading from the Dictionary in the next thread. Otherwise you would be reading a potentially stale value.

It would not be an issue on x86 where there are strong memory model guarantees but I think it is an issue on x86_x64? I stumbled on this MSDN article which states the following:

The x86-x64 processor will not reorder two writes, nor will it reorder two reads. However, the one (and only) possible reordering effect is that when a processor writes a value, that value will not be made immediately available to other processors.

Wouldn't that be a problem for the C# Dictionary which underpins IFeatureCollection?

[Tratcher]

No, await is a memory barrier. This is also not the kind of behavior you'd get with dictionary corruption, they tend to go into infinite loops or throw odd exceptions. Even if you were to break one dictionary, it wouldn't spill over to another request, they're unique objects.

It's completely plausible that identical exceptions are being thrown from your app for two requests. That's what I'd expect if the same thing went wrong twice. May makes you think those exceptions should have been different?

[aymericb]

I did not realize that await was a memory barrier. But now that I think about it, it makes sense that it would be...

Unfortunately I'm pretty sure the exceptions are the same. They're tagged with a UUID that we generate at random when sending requests to another server. When the exception occurs we serialize the request and put into the Data dictionary. So there must be a concurrency bug somewhere... just got nothing to do with IExceptionHandlerFeature.

Thanks for your help. Much appreciated!

[aymericb]

Still do not know the source of this bug, but pretty sure it is not caused by the framework.