simpleinjector / SimpleInjector

An easy, flexible, and fast Dependency Injection library that promotes best practice to steer developers towards the pit of success.
https://simpleinjector.org
MIT License
1.21k stars 153 forks source link

Prevent Microsoft.Bcl.AsyncInterfaces dependency #867

Closed dotnetjunkie closed 3 years ago

dotnetjunkie commented 3 years ago

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 commented 3 years ago

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:

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:

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 commented 3 years ago

feature-867 branch created.

dotnetjunkie commented 3 years ago

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:

This means that developers:

dotnetjunkie commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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

dotnetjunkie commented 3 years ago

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:

dotnetjunkie commented 3 years ago

Hi @davidroth and others,

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

@Tornhoof, documentation can be found here.