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

Verification fails with IAsyncDisposable registrations #873

Closed
dotnetjunkie opened this issue Dec 4, 2020 · 5 comments
Closed

Verification fails with IAsyncDisposable registrations #873

dotnetjunkie opened this issue Dec 4, 2020 · 5 comments

Comments

@dotnetjunkie
Copy link
Collaborator

dotnetjunkie commented Dec 4, 2020

To reproduce:

public class MyAsyncScoped : IAsyncDisposable
{
    public ValueTask DisposeAsync() => default;
}

var container = new Container();
container.Options.DefaultScopedLifestyle = new AsyncScopedLifestyle();
container.Register<MyAsyncScoped>(Lifestyle.Scoped);
container.Verify();

Verify fails with the following exception:

System.InvalidOperationException: 'MyAsyncScoped only implements IAsyncDisposable, but not IDisposable. Make sure to call Scope.DisposeAsync() instead of Dispose().'

Context:

  • Simple Injector v5.1.0

Solution

There are a few possible solutions:

  1. Add an VerifyAsync() method and throw an expressive exception in case Verify() is called with IAsyncDisposable registrations
  2. Let Verify() dispose the IAsyncDisposable services synchronously by calling .GetAwaiter().GetResult().
  3. Skip disposing those async disposables during verification
  4. Allow Verify() to operate in the current active scope (if any), instead of using its own scope. This prevents verify from having to call Scope.Dispose(), because disposal will be done later on.

Downsides of each solutions:

  1. Just moves the issue, because Verify is during the first call to GetInstance. Get instance is not, and cannot become async and Verify() is often called from startup code that is not, and cannot, become asynchronous (e.g. ASP.NET startup code).
  2. This might cause deadlocks on UI-driven applications (e.g. WPF, Win Forms) that have a SynchronizationContext that runs the code on the UI thread.
  3. Skipping disposal might cause those components to cause resource or memory leaks.
  4. This way delay the disposal of all (scoped) components and, therefore, testing of the dispose logic, up until the active scope is disposed. Also, if this scope happens to be a long-running scope, it will cause all scoped components to stay needlessly in memory. Also doesn't solve the problem in the initial code sample, where there is no scope. What also complicates the matter is that users can use multiple scoped lifestyles in their application. How do we retrieve the current active scope?

Our best bet is option 4. Although tracked components stay needlessly in memory, most applications will only have a handful of scoped components that will stay referenced. The risk is, therefore, low.

@dotnetjunkie dotnetjunkie added this to the Simple Injector v5.2 milestone Dec 5, 2020
@dotnetjunkie
Copy link
Collaborator Author

bug-873 branch created.

@mkostreba
Copy link

mkostreba commented Sep 23, 2022

This appears to be back in 5.4.
"'Blazorise.Modules.JSSelectModule' type only implements IAsyncDisposable. Use DisposeAsync to dispose the container.", "Data": null,
"InnerException": null,
"HelpURL": null,
"StackTraceString": " at Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceProviderEngineScope.Dispose()
at SimpleInjector.Scope.DisposeInstancesInReverseOrder(List`1 disposables, Int32 startingAsIndex)
at SimpleInjector.Scope.DisposeRecursively(Boolean operatingInException)
at SimpleInjector.Scope.Dispose(Boolean disposing)
at SimpleInjector.Container.VerifyInternal(Boolean suppressLifestyleMismatchVerification)
at SimpleInjector.Container.Verify(VerificationOption option)

This was discussed in the past here in the aspnetcore repo also: dotnet/aspnetcore#12918

@dotnetjunkie
Copy link
Collaborator Author

@mkostreba, your bug report is similar to the previous report, but has a different cause. It's, therefore, a different bug. I moved this to issue 35 of the appropriate repository. You can follow the bug's status there.

@dotnetjunkie
Copy link
Collaborator Author

See issue 35 for a solution to this problem.

@mkostreba
Copy link

mkostreba commented Sep 24, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants