Thread-affinity issue during async dispose fixed

[NorekZ]

Hello,

we encountered a bug when using IAsyncDisposable. It seems that MultitenantContainer's DisposeAsync wrongly uses ReaderWriterLockSlim in async context, because it internally compares locking and releasing thread ID. More here: https://stackoverflow.com/questions/19659387/readerwriterlockslim-and-async-await

According to aforementioned issue it seems to be the best option to use SemaphoreSlim instead of relying on custom or 3rd party implementation of AsyncReaderWriterLock.

Thank you in advance for merging.
N.

[codecov]

Codecov Report

Patch coverage: 92.50% and project coverage change: +2.56% 🎉

Comparison is base (ad17024) 88.05% compared to head (90a126d) 90.62%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop      #35      +/-   ##
===========================================
+ Coverage    88.05%   90.62%   +2.56%     
===========================================
  Files            5        5              
  Lines          134       96      -38     
  Branches        24       27       +3     
===========================================
- Hits           118       87      -31     
+ Misses          10        4       -6     
+ Partials         6        5       -1     

Files Changed	Coverage Δ
src/Autofac.Multitenant/MultitenantContainer.cs	89.15% <92.50%> (+2.37%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tillig]

I think the concept here is right, but something needs to be done about the need for multiple threads to read at the same time. How does ASP.NET do this? That StackOverflow answer, while still valid, is 10 years old. Have new approaches come up in the meantime? Does the full ReaderWriterLock have the same problems? Ideally we wouldn't take an additional dependency to solve this; perhaps ConcurrentDictionary is enough? Of course, I think we had some ConcurrentDictionary issues in core Autofac, something about it not getting properly disposed or hanging onto things incorrectly. I can't remember exactly right now.

[NorekZ]

IMO ConcurrentDictionary is by far the best option. I haven't came accross any issues with it so far.

I played a bit with atomicity and at the end I favor optimistic concurrency approach that potentially creates new LifetimeScope and then disposes it when it cannot be added to dictionary in case of concurrent call. This should almost never happen.

[chrisbbe]

A trick with ConcurrentDictionary is to use Lazy to ensure tread-safe, run once and lazy init of the tenant ILifetimeScope. i.e: ConcurrentDictionary<object, Lazy<ILifetimeScope>> = new();

[tillig]

I haven't had a chance to come back and look at this yet, but it will probably involve looking at how we dealt with async disposal in core Autofac. I do remember there was some nature of locking required on the dictionary of disposables, to ensure disposal wasn't happening at the same time as resolution. I'd like to make sure similar things are captured here, like someone isn't trying to configure a tenant at the same time as disposal or something. So "no locks at all" isn't good, at least in the "clearing everything out" sense. I also remember there was something (might be this Lazy<T> trick?) required to ensure the concurrent dictionary got cleaned up. Again, kinda on my phone right now and not looking at the code so maybe that doesn't apply here, but that's what I'll be looking at.

[tillig]

It also looks like with the removal of code here we're not passing the coverage check, which is going to block this. And that sucks, but it's there for a reason, so I'll have to look and see what's not covered and maybe suggest some tests to help boost the numbers.

[NorekZ]

I haven't had a chance to come back and look at this yet, but it will probably involve looking at how we dealt with async disposal in core Autofac. I do remember there was some nature of locking required on the dictionary of disposables, to ensure disposal wasn't happening at the same time as resolution. I'd like to make sure similar things are captured here, like someone isn't trying to configure a tenant at the same time as disposal or something. So "no locks at all" isn't good, at least in the "clearing everything out" sense. I also remember there was something (might be this Lazy<T> trick?) required to ensure the concurrent dictionary got cleaned up. Again, kinda on my phone right now and not looking at the code so maybe that doesn't apply here, but that's what I'll be looking at.

Disposal issue can be resolved using "is disposed" flag that disallows creating of new scope during and after disposal. See my next update. I use Interlocked here to ensure all threads has current version of the flag value.

[NorekZ]

A trick with ConcurrentDictionary is to use Lazy to ensure tread-safe, run once and lazy init of the tenant ILifetimeScope. i.e: ConcurrentDictionary<object, Lazy<ILifetimeScope>> = new();

Lazy trick can be done, but it potentially locks concurrent threads. IMO the suggested change will perform better in this case.

[tillig]

OK, I figured it out - it was ConcurrentBag<T> giving us trouble - if you don't call Clear on it, it doesn't get garbage collected. ConcurrentDictionary is fine. Whew!

[tillig]

This is great, thanks! I'll see about getting a release out with this shortly.

[tillig]

Oh, bah, there's a build issue.

[tillig]

Minor analyzer issue blocking the build.

[NorekZ]

Minor analyzer issue blocking the build.

I fixed the field initializer, what should I do now? I don't see it...
Thanks

[tillig]

It's failing code coverage. If you look at the list of checks, you can see codecov is what's failing. With the change in code (reduction in lines) the overall percentage has dipped below the previous coverage, which means we're short on some tests. If you click into the "Files" tab here, codecov has added some notes to show which lines aren't covered.

In particular, it looks like unit testing around the ObjectDisposedException throwing is not present - tests that validate that you can't create a new scope post-disposal.

[NorekZ]

Ok I added some tests to increase the coverage. Now all checks are passing.

[tillig]

Thanks for this, and your patience getting through the hoops. We appreciate it! I'll try to get a 0.0.1 release out with this shortly.