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

Prevent Microsoft.Bcl.AsyncInterfaces dependency #867

Closed
dotnetjunkie opened this issue Nov 12, 2020 · 10 comments
Closed

Prevent Microsoft.Bcl.AsyncInterfaces dependency #867

dotnetjunkie opened this issue Nov 12, 2020 · 10 comments

Comments

@dotnetjunkie
Copy link
Collaborator

#823 and #825 demonstrate that the new Microsoft.Bcl.AsyncInterfaces dependency that the Simple Injector core library uses is problematic.

Instead, Simple Injector should try to do duck typing so the dependency on this NuGet package can be removed.

@dotnetjunkie dotnetjunkie modified the milestone: Simple Injector v5.1.1 Nov 12, 2020
@dotnetjunkie
Copy link
Collaborator Author

dotnetjunkie commented Nov 13, 2020

Unfortunately, the dependency with both Microsoft.Bcl.AsyncInterfaces (IAsyncDisposable), and its System.Threading.Tasks.Extensions dependency (ValueTask) are very strong. Removing them would inevitably lead to breaking changes:

  • Container and Scope now implement IAsyncDisposable in .NET 4.6.1 and up, and .NET Standard 2.0 and up. Removing the dependency on Microsoft.Bcl.AsyncInterfaces will force the removal of IAsyncDisposable from Container and Scope for all Simple Injector target builds except .NET Standard 2.1. This is a breaking change for anyone using a target < .NET Standard 2.1 who depends on those types implementing IAsyncDisposable (e.g. when developers use async using).
  • To prevent the binding redirect hell we're in, System.Threading.Tasks.Extensions must be removed as well, but this means that no ValueTask can be returned. Although C# uses duck typing when calling async using, this means that the DisposeAsync methods will get a different signature, which will make different target builds incompatible and might code to break -either at compile time or at runtime- when an application starts using a different target. Especially at runtime this would lead to JIT MethodNotFound exceptions.

These breaking changes are big. Without introducing a breaking change, it would mean we would be stuck this situation until we can remove all targets prior to .NET Standard 2.1, meaning removing the:

  • .NET 4.5
  • .NET 4.6.1
  • .NET 4.8
  • .NET Standard 1.0
  • .NET Standard 1.3
  • .NET Standard 2.0

In other words, removing .NET 4.x support as it does not support .NET Standard 2.1.

Although the targets for .NET 4.5, .NET Standard 1.0, and .NET Standard 1.3 do not depend on Microsoft.Bcl.AsyncInterfaces, removing .NET 4.6.1 or .NET Standard 2.0, will cause NuGet to fall back on .NET 4.5 or .NET Standard 1.3, which should be considered a breaking change, because it would cause the DisposeAsync method to disappear from the API (or change in signature).

@dotnetjunkie
Copy link
Collaborator Author

feature-867 branch created.

@dotnetjunkie
Copy link
Collaborator Author

dotnetjunkie commented Dec 5, 2020

Developers keep hitting this issue, and I even got smacked in the face by this myself quite recently, while trying for quite some time to get the binding redirects fixed, without success. The pain is so enormous that I think this warrens a this breaking change.

This means that I'm going to make the following changes in v5.2:

  • The Microsoft.Bcl.AsyncInterfaces dependency will be removed from the Simple Injector core library (which also impacts the ASP.NET Core integration packages that need to be updated as well).
  • The .NET Standard 2.1 build of Simple Injector will behave as is, because it can still depend on IAsyncDisposable, except for the following:
    • [BREAKING] The Scope.RegisterForDisposal(IAsyncDisposable) method is replaced with Scope.RegisterForDisposal(object)
  • For the .NET Standard 2.0 build and .NET 4.6.1 build of Simple Injector, the following will change:
    • [BREAKING] Container and Scope no longer implement IAsyncDisposable (making it impossible to use C# 7.3 async using (...) any longer)
    • [BREAKING] Container and Scope no longer contain an ValueTask DisposeAsync() method
    • Container and Scope use duck typing to still allow asynchronous disposal (of course in a highly optimized fashion)
    • Container contains a new Task DisposeContainerAsync() method for asynchronous disposal
    • Scope contains a new Task DisposeScopeAsync() method for asynchronous disposal
    • A new object[] Scope.GetAllDisposables() method is added to allow retrieving both IDisposable and IAsyncDisposable instances.
    • For registering IAsyncDisposable components in the container, you can either take a dependency on Microsoft.Bcl.AsyncInterfaces, or define your own System.IAsyncDisposable interface that contains an Task DisposeAsync() method (but please do not mix these two approaches).
    • [BREAKING] The Scope.RegisterForDisposal(IAsyncDisposable) method is replaced with Scope.RegisterForDisposal(object)

This means that developers:

  • using .NET Core 3 and .NET 5 can still make use of async using while disposing Scope instances.
  • using .NET Core 2 and .NET 4 can call await Container.DisposeContainerAsync() and await Scope.DisposeScopeAsync() to do asynchronous disposal, while either taking a dependency on Microsoft.Bcl.AsyncInterfaces or define their own System.IAsyncDisposable interface to make components async disposable.
  • using .NET 4.5, .NET Standard 1.0, and .NET Standard 1.3 versions of Simple Injector are now able to use asynchronous disposal as well; they are required, however, to define their own System.IAsyncDisposable interface. Just add the following interface to your code base and Simple Injector will recognize it:
    namespace System
    {
        public interface IAsyncDisposable
        {
            Task DisposeAsync();
        }
    }

@dotnetjunkie
Copy link
Collaborator Author

dotnetjunkie commented Dec 5, 2020

5.2.0-alpha1 version of core library has been pushed to NuGet. Please respond to this thread with your comments, questions and observations.

@Tornhoof
Copy link

Tornhoof commented Dec 5, 2020

The amount of work you put into getting this to work is staggering. I'm not affected by this, but may I suggest that you include a short example (maybe in this issue and Docs) how you think devs should define their own System.IAsyncDisposable interface.

@davidroth
Copy link
Contributor

davidroth commented Dec 7, 2020

Nice work. I admire your commitment to support .NET Standard < 2.1. This issue shows that cross-targeting to support legacy frameworks adds a lot of maintenance overhead 🙈

One additional story regarding the "IAsyncDisposable" issue:

We recently created a reusable class library targeting .net5, which only referenced "SimpleInjector.Integration.GenericHost" at the beginning.
So in the initial draft, we did not explicitly reference "SimpleInjector", because that gets resolved as a transitive nuget dependency via the GenericHost package.

Later on when compiling, the error from this issue popped up:

The type 'IAsyncDisposable' is defined in an assembly that is not referenced. You must add a reference to assembly 'Microsoft.Bcl.AsyncInterfaces, Version=1.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51

Because of the way transitive resolving works, our library implicitly referenced SimpleInjector Version="5.0.0". (SimpleInjector.Integration.GenericHost references SimpleInjector 5.0.0).

The "5.0.0" package of SimpleInjector does not multi-target ".net standard 2.1". It only multi-targets ".net standard 2.0".
Since "IAsyncDisposable" isnt a thing in ".net standard 2.0", the "Microsoft.Bcl.AsyncInterfaces" package was requested by the compiler because that got exposed via the ".net standard 2.0" targeted SimpleInjector v 5.0.0.

SimpleInjector >= "5.0.1", does multi-target for ".net standard 2.1". So no more Microsoft.Bcl.AsyncInterfaces needed beceause IAsyncDisposable is a thing in ".net standard 2.1".

The fix was easy and obvious:
We explicitly added a reference to SimpleInjector "5.1.0" and the error was gone.

Therefore I'd suggest publishing new versions of the Integration packages as well because they can cause this error when "older" transient references are used. The updated integration packages should at least reference SI 5.0.1, because thats the point where SI started targeting ".net standard 2.1".

@dotnetjunkie
Copy link
Collaborator Author

@davidroth,

Therefore I'd suggest publishing new versions of the Integration packages as well because they can cause this error when "older" transient references are used.

We used to do this prior to v5, but decided to stop doing this in v5. As you noticed, however, this proves to be very problematic due to the way NuGet works. There is a longstanding issue with the NuGet team, that just won't get fixed by them, and the problem only got worse with recent Visual Studio NuGet plugins. This is why I actually decided to partly go back to old situation, as described here.

What this means is that the ASP.NET Core integration packages are considered one and will always be updated together. They will still reference the lowest Simple Injector core library version possible. Since there is a problem in core library v5.0.0, however, those integration packages shouldn't reference that library, and should have been updated. With the introduction of v5.2 we will update all NuGet packages, so hopefully the problem goes away completely.

@davidroth
Copy link
Contributor

Thanks for sharing the links!
Yes, the current situation regarding transient nuget dependencies is indeed suboptimal.
Anyway, thanks for updating the packages 👍

@dotnetjunkie
Copy link
Collaborator Author

dotnetjunkie commented Dec 8, 2020

The breaking change in the 5.2.0 alpha release broke (all) the ASP.NET Core integration packages; which is something that was expected. For the last days, I've been working on an alpha release of those as well. Those are now pushed to NuGet.

To everybody reading this, please try them out and provide me with feedback below.

NOTE: Those integration packages still handle asynchronous disposal of request scopes and asynchronous disposal of the container at application shutdown. I expect users of the integration packages not having to make any changes to their applications, except in cases where they interact directly with Scope instances, for instance because of starting background operations.

Interesting things to test:

  • Do you get any exceptions, or experience any problems after upgrading your application to these latest alpha releases?
  • If you got any exceptions, was the exception message clear enough for you to fix the problem?
  • If you have any IAsyncDisposable registrations (scoped and/or singleton), are those disposed of?
  • Does your application shutdown gracefully? (tip: this post explains how to trigger a shutdown)

@dotnetjunkie
Copy link
Collaborator Author

Hi @davidroth and others,

Simple Injector v5.2 has been released today with fixes the binding redirect issues.

@Tornhoof, documentation can be found here.

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

3 participants