Update DI and Hosting to use Microsoft.Bcl.AsyncInterfaces #2137
Conversation
kevinchalet
requested review from
analogrelay,
dougbu,
Tratcher and
a team
as code owners
There was a problem hiding this comment.
The code changes look good. @davidfowl @anurse who would know what the plans are for Microsoft.Bcl.AsyncInterfaces to see if it meets our requirements?
|
This is a good change but we don’t want any unnecessary package refs by default on netcoreapp3.0 which means we’ll need to multi target in more places (like hosting). @JunTaoLuo was working on that. |
|
@davidfowl both the hosting abstractions and implementations packages already target AFAICT, most of the packages here reference The only stack that uses Would you like me to do that? |
|
@JunTaoLuo is already adding multi-targeting in https://github.com/aspnet/Extensions/pull/2090/files#diff-e607480dfcb295da19cb5dc533726055. |
|
Perfect, I'll leave this PR as-is, then. Let me know if there's anything I help with. |
|
@Tratcher @davidfowl to be clear, this hasn't yet been submitted for 3.0 approval. Please make sure to check in with me before merging anything. I do have concerns about the churn this will create in 3.0 and the very short runway we have to make sure we get this right. |
|
@anurse it's your call, but I'd be interested in reading more about your concerns. To be clear, this PR doesn't add new code, it simply realigns the .NET Standard source with the .NET Core one by using the BCL async package and removing conditional compilation constants that previously excluded async disposal support from .NET Standard. It's exactly what you guys did in ASP.NET Core with the connections abstractions and the SignalR .NET client (which is why I assumed it was an oversight). It's also worth mentioning this area was already covered by many tests, that will now ensure async disposal works just fine on .NET Framework. |
It's not actually my call, but it is indeed our call ;).
We implemented In general, I agree that this change makes sense and was probably just an oversight. We just need to work through the risk. For example, an issue just arose from this discussion (though it's not directly relevant to this PR and exists in the |
|
Roger.
Nice. Glad it helped find a bug there 😄 |
|
See #2090. This is in the same bucket and is less risky than the linked PR |
|
Looks like there are merge conflicts. Do you want me to rebase this PR? |
|
@davidfowl @anurse did you guys eventually come to an agreement? 😄 |
|
@PinpointTownes they both happen to be out right now, but I'll get back to you by Monday at the latest. Sorry for the delay. |
|
Thanks @Pilchie! No hurry, I just wanted to make sure this was still considered for the final 3.0 release, since it enables an important scenario (i.e async disposables on .NET Standard 2.0-only platforms like .NET Framework). |
|
@Pilchie are we still planning on getting this into 3.0? cc @aspnet/build for awareness. |
|
@JunTaoLuo in the same vein: dotnet/aspnetcore#13343 (I'm pretty sure the Extensions repo also has problematic calls) |
|
Yes we should look at getting this for 3.0. |
|
Rebased and resolved the conflicts in the props files 😄 |
|
Hum, the CI failure looks like a flaky test 🤕 |
|
I'm taking a closer look at the downstream effects and I see a few more packages that are affected:
I see quite a few instances where these affected packages target netstandard2.1 so I'm thinking that if we cross-target M.E.Hosting and M.E.DependencyInjection with netstandard2.1 we should resolve most of these issues without requiring additional changes in downstream packages. Thoughts @davidfowl ? |
|
@PinpointTownes Can you make the changes I mentioned in the previous comment? We need to additionally cross-target netstandard2.1 for M.E.Hosting, M.E.DependencyInjection and M.E.Hosting.WindowsServices. The last one is needed to ensure the dependency isn't referenced in an app that references it running on core. |
|
@JunTaoLuo I'm OOO today but feel free to directly push that to my fork so it's added to this PR. Otherwise, I'll take a look tonight 😄 |
|
Note: Do to the expanding scope of this and lack of runway, we decided NOT to take this for 3.0, but we will plan to take it for 3.1 instead. I believe @anurse had a couple of concerns with places we might have other bugs around async disposal too. |
|
That's extremely unfortunate. Are you at least going to update DI to dispose |
Not for 3.0 at this point. |
|
Thanks @PinpointTownes! Unfortunately, this will only be available starting in 3.1 |
|
Thanks! |
…tensions#2137) * Update DI to use Microsoft.Bcl.AsyncInterfaces for the net461 and netstandard20 TFMs * Update Hosting to use Microsoft.Bcl.AsyncInterfaces for the netstandard20 TFM * Make sure async disposal tests are executed on .NET Framework * Multitarget more projects Commit migrated from dotnet/extensions@1264b9e
…tensions#2137) * Update DI to use Microsoft.Bcl.AsyncInterfaces for the net461 and netstandard20 TFMs * Update Hosting to use Microsoft.Bcl.AsyncInterfaces for the netstandard20 TFM * Make sure async disposal tests are executed on .NET Framework * Multitarget more projects Commit migrated from dotnet/extensions@1264b9e
…tensions#2137) * Update DI to use Microsoft.Bcl.AsyncInterfaces for the net461 and netstandard20 TFMs * Update Hosting to use Microsoft.Bcl.AsyncInterfaces for the netstandard20 TFM * Make sure async disposal tests are executed on .NET Framework * Multitarget more projects Commit migrated from dotnet/extensions@1264b9e
…tensions#2137) * Update DI to use Microsoft.Bcl.AsyncInterfaces for the net461 and netstandard20 TFMs * Update Hosting to use Microsoft.Bcl.AsyncInterfaces for the netstandard20 TFM * Make sure async disposal tests are executed on .NET Framework * Multitarget more projects Commit migrated from dotnet/extensions@1264b9e
ghost
locked as resolved and limited conversation to collaborators
Fixes #1467.
/cc @Tratcher @anurse (I believe you own the Hosting stack)
/cc @davidfowl @Eilon (not sure who's responsible of DI these days 😄)