Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding IAsyncDisposable Support to Autofac #1037

Merged
merged 8 commits into from Oct 20, 2019

Conversation

@alistairjevans
Copy link
Member

alistairjevans commented Oct 18, 2019

I've made what I believe are the necessary changes to make asynchronous disposal of lifetime scopes work. See https://docs.microsoft.com/en-us/dotnet/api/system.iasyncdisposable?view=dotnet-plat-ext-3.0 for some background.

This now works:

await using (var scope = container.BeginLifetimeScope())
{
   var service = scope.Resolve<ServiceThatImplementsIAsyncDisposable>();
   // When the scope disposes, any services that implement IAsyncDisposable will be 
   // Disposed of using DisposeAsync rather than Dispose.
}

I've added framework targeting for netcoreapp3.0, and made all the IAsyncDisposable support conditional on that target.

The main changes for this are in the Disposer, which has to now handle async dispose of all registered objects. The locking mechanism to sync access to the set of tracked objects has changed from a lock statement to a SemaphoreSlim, because the Monitor does not work in an async context. Both IDisposable items and IAsyncDisposable items can be added to the same Disposer, and they will be disposed in the same order as they would have been with normal Dispose. Objects that only have an IAsyncDisposable implementation will cause an exception to be thrown if a LifetimeScope is then disposed of synchronously. This matches the behaviour of the main .NET DI system.

The Disposable base class has been updated to implement IAsyncDisposable, and provide default functionality of calling synchronous Dispose if an implementing class does not need to support DisposeAsync. Container and LifetimeScope do implement DisposeAsync, so they can Dispose of the root lifetime scope and the scope's Disposer respectively.

There's an associated set of changes needed in the DependencyInjection integration, which I have locally, to allow the Autofac lifetime scope to get cleaned up async on scope end, by implementing IAsyncDisposable on the relevant framework types.

I've added a selection of tests to check that async behaviour works as expected, and plays nicely with normally-disposed and async-disposed objects.

Alistair Evans and others added 3 commits Oct 18, 2019
src/Autofac/Autofac.csproj Outdated Show resolved Hide resolved
@alsami

This comment has been minimized.

Copy link
Member

alsami commented Oct 19, 2019

@alistairjevans

Would it make sense to add the support for IAsyncDisposable in the library Autofac.Extension.DependencyInjection and Autofac.AspNetCore.Multitenant?

The default Microsoft-Service-Provider also provides this functionality as can be seen here and is actually implemented somewhere deep down in ServiceProviderEngineScope.

We have two different implementations of IServiceProvider there as well. One is AutofacServiceProvider which is the default and AutofacMultitenantServiceProvider which derives from AutofacServiceProvider.

If so, it would probably be needed in the MultitenantContainer as well.

@alistairjevans

This comment has been minimized.

Copy link
Member Author

alistairjevans commented Oct 19, 2019

@alsami

Yes, it absolutely should, the support for this in ASP.NET Core was actually my main reason for introducing these changes. :)

I've already done the Autofac.Extension.DependencyInjection changes because I wanted to prove compatibility in ASP.NET core, you can see the changes in my fork branch:
autofac/Autofac.Extensions.DependencyInjection@develop...alistairjevans:async-dispose

I haven't raised a PR for that yet because it'll wait until there's a pre-release package with the changes from this PR.

I believe that simply deriving from the AutofacServiceProvider will grant the AutofacMultitenantServiceProvider async dispose support.

It is worth noting (as I discovered) that it is the IServiceScope, rather than the IServiceProvider that gets DisposeAsync called by the framework.

Changes will be needed in the Autofac.MultiTenant integration, to enable async dispose on the MultitenantContainer (and the nested tenant scopes), however even without that, any nested lifetime scopes resolved from that container will be async-disposable.

@alsami

This comment has been minimized.

Copy link
Member

alsami commented Oct 19, 2019

Yes, it absolutely should, the support for this in ASP.NET Core was actually my main reason for introducing these changes. :)

Okay cool :)

I will take on the changes required for the MultitenantContainer, if you don't mind. As @davidfowl pointed out, there is no need to explicitly add support for netcoreapp3.0 since the library Microsoft.Bcl.AsyncInterfaces adds support for IAsyncEnumerable and IAsyncDisposable (and ValueTask through explicit reference to System.Threading.Tasks.Extensions) to several framework monikers lower than netstandard2.1 and netcoreapp3.0.

…dded conditional package reference for netstandard2.0.
@alistairjevans

This comment has been minimized.

Copy link
Member Author

alistairjevans commented Oct 19, 2019

@alsami, yes, perfectly happy for you to take on the multitenant changes!

I've changed Autofac to target netstandard2.1 instead of netcoreapp3.0, and added the Microsoft.Bcl.AsyncInterfaces @davidfowl mentioned for netstandard2.0 only.

Also changed all the #if NETCOREAPP3_0 conditions to a new define, ASYNC_DISPOSE_AVAILABLE.

@alsami

This comment has been minimized.

Copy link
Member

alsami commented Oct 19, 2019

Also changed all the #if NETCOREAPP3_0 conditions to a new define, ASYNC_DISPOSE_AVAILABLE.

Since this would be part of the 5.0 release, wouldn't it make sense to drop support for net451 and netstandard1.1 and add net461 if it is really required? From my understanding we then wouldn't need any compiler-directives since Microsoft.Bcl.AsyncInterfaces supports net461 explicitly.

@alexmg and @tillig, would that be a possibility?

@alistairjevans

This comment has been minimized.

Copy link
Member Author

alistairjevans commented Oct 19, 2019

@alsami, I understand the desire to drop support for net45 and that would be nice...

However, there will be people out there (including myself sadly) who work with a couple of projects that can't move from 4.5.2 for various reasons, and it seems harsh to deprive those projects of future upgrades of Autofac just for the sake of a few compiler directives.

Happy to go with the owner ruling on this though whichever way it goes.

@alsami

This comment has been minimized.

Copy link
Member

alsami commented Oct 19, 2019

who work with a couple of projects that can't move from 4.5.2 for various reasons

If that is the case I totally agree with you. To be honest I never came across a project that couldn't be updated from 4.x to 4.6.1.

I always dream about not having to boot into my windows virtual machine to edit some .NET-project 🤣

@alexmg

This comment has been minimized.

Copy link
Member

alexmg commented Oct 20, 2019

I think we should drop net45 for the 5.0 release. Replacing that with net461 means the ASYNC_DISPOSE_AVAILABLE constant and associated #if can be removed. Seeing those #if in the middle of fundamental parts of the code makes me uneasy. We have been overly generous with our net45 support and now is certainly an appropriate time to make a trade-off in the name of maintainability. We are talking about a framework version that is long out of support. For those that are stuck on older framework versions our existing 4.9 versions will continue to serve them well.

…if blocks for async dispose support.
…etadata> reference problems and update/remove other incompatible #if statements after the targeting changes.
@alistairjevans

This comment has been minimized.

Copy link
Member Author

alistairjevans commented Oct 20, 2019

@alexmg, I've updated the changes so Autofac now targets net461 instead of net45, and removes all the #if conditions I had before. Always happy to defer to you guys on the best compatibility choices!

Initially I only had netstandard2.0 and netstandard2.1, but then I checked the docs and MS suggest having net461 as well, so that went back in. Also had to update other #if conditions and fix Lazy<T, TMetadata> problems.

Copy link
Member

alexmg left a comment

This is looking good. Removing net45 definitely reduced the noise. Please use await in the DisposerTests and this is good to go.

test/Autofac.Test/Core/DisposerTests.cs Show resolved Hide resolved
@alexmg alexmg added this to the v5.0 milestone Oct 20, 2019
@alexmg alexmg added the enhancement label Oct 20, 2019
@alexmg
alexmg approved these changes Oct 20, 2019
@alexmg alexmg merged commit 9eee0ea into autofac:develop Oct 20, 2019
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@alexmg

This comment has been minimized.

Copy link
Member

alexmg commented Oct 20, 2019

Nice contribution @alistairjevans! 👏

This is now available in the Autofac.5.0.0-develop-00622 package on MyGet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.