CODEWITHSUNDEEP
c#asp.netmultithreadingasp.net-web-apiautofac
rexcfnghk
rexcfnghk

Reputation: 15482

Thread synchronization in DI container managed instances

I have a FooContext class that captures some HTTP request-specific runtime values from the request inside an ASP.NET Web API application

public class FooContext
{
    private readonly ISet<string> _set = new HashSet<string>();

    public void AddToSet(string s) => _set.Add(s);

    // Copied so that caller won't modify _set
    public ISet<string> GetStrings() => new HashSet<string>(_set);
}

Multiple consumers depends on this FooContext and will call AddToSet/GetStrings and depending on the result, run different business logic.

I want to guarantee there will only be one instance per of FooContext per HTTP request so I registered inside the DI container as request-scoped (using Autofac here as an example but I guess most containeirs are roughly the same):

protected override void Load(ContainerBuilder builder)
{
    builder.RegisterType<FooContext>().InstancePerRequest();
}

My understanding is, FooContext is not thread-safe because threads may call GetStrings/AddToSet at the same time on the same FooContext instance (since it is request-scoped). It is not guaranteed that each HTTP request will complete on one single thread.

I do not explictly create new threads nor call Task.Run() in my application but I do use a lot of async-await with ConfigureAwait(false), which means the continuation may be on a different thread.

My questions are:

  1. Is it true that FooContext is not thread-safe? Is my understanding above correct?
  2. If this is indeed thread unsafe, and I want to allow multiple readers but only one exclusive writer, should I apply a ReaderWriterLockSlim on the ISet<string>?

Update

Since a commenter comments that my question is unanswerable without showing FooContext's usage, I will do it here. I use FooContext in an IAutofacActionFilter to capture several parameters that are being passed in the controller method:

public class FooActionFilter : IAutofacActionFilter
{
    private readonly FooContext _fooContext;

    public FooActionFilter(FooContext fooContext)
        => _fooContext = fooContext;

    public Task OnActionExecutingAsync(
        HttpActionContext actionContext, 
        CancellationToken cancellationToken)
    {
        var argument = (string)actionContext.ActionArguments["mystring"];
        _fooContext.AddToSet(argument);
        return Task.CompletedTask;
    }
}

Then in other service classes that control business logic:

public class BarService
{
    private readonly FooContext _fooContext;

    public BarService(FooContext fooContext)
        => _fooContext = fooContext;

    public async Task DoSomething()
    {
         var strings = _fooContext.GetStrings();
         if (strings.Contains("foo"))
         {
             // Do something
         }
    }
}

Upvotes: 0

Views: 289

Answers (2)

Steven
Steven

Reputation: 172716

It is not guaranteed that each HTTP request will complete on one single thread.

When using async/await, the request might run on multiple threads, but the request will flow from one thread to the other, meaning that the request will not run on multiple threads in parallel.

This means that the classes that you cache on a per-request basis don't have to be thread-safe, since their state is not accessed in parallel. They do need to be able to be accessed from multiple threads sequentially though, but this is typically only a problem if you start storing thread-ids, or any other thread-affinit state (using ThreadLocal<T> for instance).

So don't do any special synchronization or use any concurrent data structures (such as ConcurrentDictionary), because it will only complicate your code, while this is not needed (unless you forget to await for some operation, because in that case you will accidentally run operations in parallel, which can cause all sorts of problems. Welcome to beautiful world of async/await).

Multiple consumers depends on this FooContext and will call AddToSet/GetStrings and depending on the result, run different business logic.

Those consumers can depend on FooContext as long as they have a lifetime that is either Transient or InstancePerRequest. And this holds for their consumers all the way up the call graph. If you violate this rule, you will have a Captive Dependency, which may cause your FooContext instance to be reused by multiple requests, which will cause concurrency problems.

You do have to take some care when working with DI in multi-threaded applications though. This documentation does give some pointers.

Upvotes: 3

Stephen Cleary
Stephen Cleary

Reputation: 456777

I do not explictly create new threads nor call Task.Run() in my application but I do use a lot of async-await with ConfigureAwait(false), which means the continuation may be on a different thread.

Yes, so it is being accessed in a multithreaded fashion.

Is it true that FooContext is not thread-safe? Is my understanding above correct?

Yes.

I would say that the calling code is actually in error: ConfigureAwait(false) means "I don't need the request context", but if the code uses FooContext (part of the request context), then it does need the request context.

If this is indeed thread unsafe, and I want to allow multiple readers but only one exclusive writer, should I apply a ReaderWriterLockSlim on the ISet?

Almost certainly not. A lock would probably work just fine.

Using reader/writer locks just because some code does reads and other does writes is a common mistake. Reader/writer locks are only necessary when some code does reads, other does writes, and there is a significant amount of concurrent read access.

Upvotes: 2

Related Questions