Ensure that IAsyncDisposable instances that have not been activated are correctly disposed when the scope is disposed. #1285

alistairjevans · 2021-08-21T11:09:04Z

This change was slightly larger than expected; due to the somewhat viral nature of async code, I had to add DisposeAsync methods in a number of places.

This necessitated the addition of IAsyncDisposable as a base interface on some of our public types. However, the types modified are IComponentRegistration and IComponentRegistry, which fall into the category of "things that people probably won't implement themselves."

I avoided a breaking change on IInstanceActivator (perhaps more likely to be implemented) by checking if it's an IAsyncDisposable before continuing.

…e instances when we dispose of scopes.

codecov · 2021-08-21T11:14:09Z

Codecov Report

Merging #1285 (e4a4a3d) into develop (79bfeef) will increase coverage by 0.09%.
The diff coverage is 87.23%.

@@             Coverage Diff             @@
##           develop    #1285      +/-   ##
===========================================
+ Coverage    76.54%   76.64%   +0.09%     
===========================================
  Files          188      188              
  Lines         5125     5163      +38     
  Branches      1042     1057      +15     
===========================================
+ Hits          3923     3957      +34     
- Misses         707      708       +1     
- Partials       495      498       +3

Impacted Files	Coverage Δ
src/Autofac/Core/Registration/ComponentRegistry.cs	`86.36% <50.00%> (-8.09%)`	⬇️
...stration/ComponentRegistrationLifetimeDecorator.cs	`70.83% <66.66%> (-4.17%)`	⬇️
...Autofac/Core/Registration/ComponentRegistration.cs	`76.74% <71.42%> (-0.48%)`	⬇️
...tors/ProvidedInstance/ProvidedInstanceActivator.cs	`94.87% <100.00%> (+2.87%)`	⬆️
src/Autofac/Core/Container.cs	`68.96% <100.00%> (ø)`
src/Autofac/Core/Disposer.cs	`95.91% <100.00%> (+0.08%)`	⬆️
...ofac/Core/Registration/ComponentRegistryBuilder.cs	`80.64% <100.00%> (+0.98%)`	⬆️
...e/Registration/DefaultRegisteredServicesTracker.cs	`84.44% <100.00%> (+0.59%)`	⬆️
src/Autofac/Util/Disposable.cs	`75.00% <0.00%> (+12.50%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 79bfeef...e4a4a3d. Read the comment docs.

tillig

No joke on the viral nature of async stuff! Looks pretty good, a couple of minor comments/questions.

tillig · 2021-08-23T14:04:19Z

src/Autofac/Core/Activators/ProvidedInstance/ProvidedInstanceActivator.cs

+                {
+                    // Type only implements IAsyncDisposable, which is not valid if there
+                    // is a synchronous dispose being done.
+                    throw new InvalidOperationException(string.Format(


I'm somewhat torn here. I'm curious if we should throw or just do like a DisposeAsync().GetAwaiter().Wait(). Or maybe nothing?

I'm not sure about throwing during Dispose, like how WCF would do if a network connection dropped and we ended up having to work around that with a special OnRelease handler. I know I'm using ServiceBusClient, which is only IAsyncDisposable, in some of our code and an exception here would be both unexpected and possibly not easy to fix, especially if the container is getting disposed by something outside my control during app shutdown.

So, this "throw if you're service isn't going to be disposable" was the approach we took for other service types when we added IAsyncDisposable support.

I suppose the problematic scenario is the one where you aren't implementing the component, like ServiceBusClient, so can't remedy the problem.

We can do the GetAwaiter().Wait(), it just means that user's might not be getting the nice async dispose they think they're getting. I too am torn.

Yeah, I didn't think the exception was going to be an issue until I hit this ServiceBusClient thing where it's causing all sorts of weird sync/async disposal problems because they didn't implement both. I have to imagine there are other cases like this out there, I just happen to find a concrete pain in my ass right here so I'm latching onto it.

I guess it comes down to: would it be better to dispose synchronously or not dispose at all? And if it doesn't dispose (and throws), it looks like it stops everything else from being disposed, too. Which I'll admit sounded right at the time but IAsyncDisposable was kind of new and I didn't really grok it like I'm starting to now.

It's almost like it should be a warning rather than an exception. Not that that's a thing, just like, "Hey, you did something wrong and we'll recover for you by doing it synchronously, but you're not getting the nice async you think you are."

Barring that... my gut is telling me we should do what we can to dispose without throwing.

Another option might be to require folks to opt in with something like

builder.RegisterType<ServiceBusClient>().SynchronousDisposal();

where SynchronousDisposal could be an OnRelease thing sort of like the WCF SafeRelease extension where it wraps DisposeAsync().GetAwaiter().Wait() logic.

What's the "pit of success" here? What will make people more readily successful with the least amount of client work? Successful by default?

Minor addition, we would need to AsTask() on the result of DisposeAsync, you cannot GetAwaiter() on a ValueTask. Only worth mentioning as another allocation that would happen during dispose.

Like you say, raising a warning would be nice. I think the pit of success is "probably" GetAwaiter().Wait(), but perhaps we could write something somewhere, like to Debug.Trace?

I'd go for a Debug.WriteLine or something similar. Maybe Trace instead of Debug - doesn't Debug get compiled out in release mode? I vaguely recall that being a thing. I could see possibly raising an event with the diagnostic listener thing for issues like this, too, maybe... but that'd mean propagating it around down to this level and yeahno.

Ok, this is done; used Trace.TraceWarning. Attached tracers understand it's a warning level, which is good.

src/Autofac/Core/Activators/ProvidedInstance/ProvidedInstanceActivator.cs

src/Autofac/Core/Registration/ComponentRegistryBuilder.cs

test/Autofac.Specification.Test/Lifetime/AsyncDisposeProvidedInstanceTests.cs

test/Autofac.Specification.Test/Util/AsyncOnlyDisposeTracker.cs

…but with a Trace warning.

tillig

📱

tillig · 2021-08-25T14:13:58Z

It looks like the build is hanging on the unit tests in netcoreapp3.1 for Autofac.Test. I'm not sure what's up, it gets past the Autofac.Specification.Test without issue but hangs up for Autofac.Test.

tillig · 2021-08-25T14:19:24Z

I can't replicate the hang locally. I wonder if updating the global.json to the latest SDK versions would have any effect. I used the build.ps1 script so it installed the specified versions, but I'm guessing the latestFeature thing allowed it to slip forward to use my later installed version instead of forcing the older version.

alistairjevans · 2021-08-25T14:21:08Z

I'll stop the build and change the global.json.

alistairjevans · 2021-08-25T16:32:49Z

That same path separator problem needed applying to the main Autofac repo, so including it in this PR.

tillig

🖱️

alistairjevans · 2021-08-25T16:50:28Z

Still hanging.

These deadlock issues remind me of one of the reasons why I didn't do this sync-over-async in the first place.

https://blog.stephencleary.com/2012/07/dont-block-on-async-code.html

I tried ConfigureAwait(false), but to no avail.

Worth noting that a deadlock is far harder to understand/debug than an exception....

alistairjevans · 2021-08-25T16:50:59Z

Interestingly only hangs on net5; perhaps some threading thing has been changed.

tillig · 2021-08-25T16:57:38Z

Hmmmm. Agreed that exception is easier to troubleshoot. I also have to admit I don't really understand why disposal needs to be async at all but OOOOOKAY. Waiting on async constructors now because that'll be the best idea EVAR. 😩

I think my biggest issue with the exception was that we didn't allow stuff to be disposed that could be disposed before throwing.

On the other hand, if I wanted to bridge the gap and put some sort of extension that allows sync disposal of an only async component, I'd still be stuck with this issue and it'd be entirely outside my control - thing X (ServiceBusClient) does async disposal but thing Y (BackgroundService) only does sync disposal. 💥

alistairjevans · 2021-08-25T17:01:09Z

🐔 <-> 🥚

alistairjevans · 2021-08-25T17:04:55Z

Async disposal is useful when you want compact cleanup of remote resources.

E.g.

await using (var resource = await CreateTemporaryResourceViaApiAsync())
{
   // resource is auto cleaned up when you're done, regardless of exceptions and whatnot.
}

Sort of besides the point though, IAsyncDisposable is here to stay, so we'd better support it.

I'll take a look and see what else we can do here.

alistairjevans · 2021-08-28T09:22:32Z

OK; fixed the deadlock via the somewhat nuclear option of bumping the Disposal of IAsyncDisposable-only objects onto the threadpool and waiting for completion, to ensure we get a completely fresh async context.

Basically, if I just call ConfigureAwait(false).GetAwaiter().GetResult() from the current thread, every method called by the disposing type will also need ConfigureAwait(false) to avoid deadlocks, which is impossible to enforce.

tillig

🤯

tillig · 2021-08-28T15:25:54Z

I can't think of anything better. Luckily it should be very rare. Nice work figuring it out!

src/Autofac/Core/Activators/ProvidedInstance/ProvidedInstanceActivator.cs

alistairjevans added 4 commits August 21, 2021 10:33

Add tests to confirm autofac#1284

9d86367

Fix autofac#1284 - Make sure we dispose non-activated IAsyncDisposabl…

b52aba2

…e instances when we dispose of scopes.

Avoid the need to modify IInstanceActivator.

60c1d9a

Remove unused namespace

07ce218

alistairjevans linked an issue Aug 21, 2021 that may be closed by this pull request

IAsyncDisposable registered via RegisterInstance does not get disposed if it was never resolved #1284

Closed

Up test coverage

1f2492e

tillig requested changes Aug 23, 2021

View reviewed changes

alistairjevans added 2 commits August 24, 2021 18:12

Fix test and apply common comment

cd76a58

Change sync disposal of IAsyncDisposable-only objects to be allowed, …

3b6f684

…but with a Trace warning.

tillig previously approved these changes Aug 25, 2021

View reviewed changes

Use latest .net5 SDK

e9b2cd5

alistairjevans dismissed tillig’s stale review via e9b2cd5 August 25, 2021 14:22

Add path separator fix

bcd176e

tillig previously approved these changes Aug 25, 2021

View reviewed changes

Do sync-over-async dispose with ConfigureAwait(false) to avoid deadlocks

f2f0d04

alistairjevans dismissed tillig’s stale review via f2f0d04 August 25, 2021 16:41

Try bumping the whole dispose operation onto the threadpool.

e4a4a3d

tillig approved these changes Aug 28, 2021

View reviewed changes

alistairjevans merged commit 6c6c7c0 into autofac:develop Aug 28, 2021

tillig mentioned this pull request Jan 18, 2022

Is the important part of the IAsyncDisposable section in the disposal.rst still true? autofac/Documentation#154

Closed

OwnageIsMagic reviewed Sep 12, 2022

View reviewed changes

src/Autofac/Core/Activators/ProvidedInstance/ProvidedInstanceActivator.cs Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure that IAsyncDisposable instances that have not been activated are correctly disposed when the scope is disposed. #1285

Ensure that IAsyncDisposable instances that have not been activated are correctly disposed when the scope is disposed. #1285

alistairjevans commented Aug 21, 2021

codecov bot commented Aug 21, 2021 •

edited

Loading

tillig left a comment

tillig Aug 23, 2021

alistairjevans Aug 24, 2021

tillig Aug 24, 2021

alistairjevans Aug 24, 2021

alistairjevans Aug 24, 2021

tillig Aug 24, 2021

alistairjevans Aug 25, 2021

tillig left a comment

tillig commented Aug 25, 2021

tillig commented Aug 25, 2021

alistairjevans commented Aug 25, 2021

alistairjevans commented Aug 25, 2021

tillig left a comment

alistairjevans commented Aug 25, 2021

alistairjevans commented Aug 25, 2021

tillig commented Aug 25, 2021

alistairjevans commented Aug 25, 2021

alistairjevans commented Aug 25, 2021

alistairjevans commented Aug 28, 2021

tillig left a comment

tillig commented Aug 28, 2021

Ensure that IAsyncDisposable instances that have not been activated are correctly disposed when the scope is disposed. #1285

Ensure that IAsyncDisposable instances that have not been activated are correctly disposed when the scope is disposed. #1285

Conversation

alistairjevans commented Aug 21, 2021

codecov bot commented Aug 21, 2021 • edited Loading

Codecov Report

tillig left a comment

Choose a reason for hiding this comment

tillig Aug 23, 2021

Choose a reason for hiding this comment

alistairjevans Aug 24, 2021

Choose a reason for hiding this comment

tillig Aug 24, 2021

Choose a reason for hiding this comment

alistairjevans Aug 24, 2021

Choose a reason for hiding this comment

alistairjevans Aug 24, 2021

Choose a reason for hiding this comment

tillig Aug 24, 2021

Choose a reason for hiding this comment

alistairjevans Aug 25, 2021

Choose a reason for hiding this comment

tillig left a comment

Choose a reason for hiding this comment

tillig commented Aug 25, 2021

tillig commented Aug 25, 2021

alistairjevans commented Aug 25, 2021

alistairjevans commented Aug 25, 2021

tillig left a comment

Choose a reason for hiding this comment

alistairjevans commented Aug 25, 2021

alistairjevans commented Aug 25, 2021

tillig commented Aug 25, 2021

alistairjevans commented Aug 25, 2021

alistairjevans commented Aug 25, 2021

alistairjevans commented Aug 28, 2021

tillig left a comment

Choose a reason for hiding this comment

tillig commented Aug 28, 2021

codecov bot commented Aug 21, 2021 •

edited

Loading