[Mvc] Support IAsyncDisposable for controllers and pages #29313
Conversation
ghost
added
the
area-mvc
There was a problem hiding this comment.
This looks greats. Are you planning to update this for view components:
- https://github.com/dotnet/aspnetcore/blob/master/src/Mvc/Mvc.ViewFeatures/src/ViewComponents/IViewComponentFactory.cs
- https://github.com/dotnet/aspnetcore/blob/master/src/Mvc/Mvc.ViewFeatures/src/ViewComponents/IViewComponentActivator.cs
It might also be helpful to throw if an IView instance implements IAsyncDisposable rather than silently no-op. It's invoked in sync code-paths and supporting IAsyncDisposable is far too hard there. (e.g. https://github.com/dotnet/aspnetcore/blob/master/src/Mvc/Mvc.ViewFeatures/src/TemplateRenderer.cs#L138)
|
@pranavkm those are great points. I was mainly testing the waters to root out any big blockers. The part that I'm more concerned about is the extensibility, but I think the approach I followed offers a reasonable trade-off. I think we should consider marking the existing sync releaser members as obsolete to hint authors to implement the async capable versions. |
javiercn
force-pushed
the
javiercn/mvc-async-disposable
branch
from
32c4a6c to
ce4c26b
Compare
| { | ||
| return Task.FromException(scopeException!); | ||
| } | ||
| return ReleaseResourcesCore(scope); |
There was a problem hiding this comment.
I refactored this code because it wasn't consistent depending on whether the path was synchronous or async or whether the values were being logged.
src/Mvc/Mvc.RazorPages/test/Infrastructure/DefaultPageFactoryProviderTest.cs
Outdated
Show resolved
Hide resolved
* Adds async versions of Release to the relevant interfaces in different factories and activators. * The new methods offer a default interface implementation that will delegate to the syncrhonous version and just call dispose. * Our implementations will override the default implementation and handle disposing IAsyncDisposable implementations. * Our implementations will favor IAsyncDisposable over IDisposable when both interfaces are implemented. * Extenders will have to override the new methods included to support IAsyncDisposable instances.
javiercn
force-pushed
the
javiercn/mvc-async-disposable
branch
from
ce4c26b to
57c353a
Compare
|
new API :| |
|
Hi @pranavkm. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context. |
|
@pranavkm It's life 😃 I've added it to the API review next week |
|
Hi @javiercn. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context. |
javiercn
added
the
api-ready-for-review
|
Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:
|
| @@ -20,5 +22,16 @@ public interface IControllerActivator | |||
| /// <param name="context">The <see cref="ControllerContext"/> for the executing action.</param> | |||
| /// <param name="controller">The controller to release.</param> | |||
| void Release(ControllerContext context, object controller); | |||
There was a problem hiding this comment.
Obsolete and never delete.
pranavkm
added
api-approved
[Fixes #13150]
*Adds async versions of Release to the relevant interfaces in different
factories and activators.
delegate to the syncrhonous version and just call dispose.
disposing IAsyncDisposable implementations.
both interfaces are implemented.
IAsyncDisposable instances.
The general pattern is that there is a new method on the relevant interfaces that offers a default implementation that delegates to the sync version in the interface. The idea is that extenders will have to do work to support IAsyncDisposable too, but this way we avoid having a breaking change and libraries can add support at their own pace by overriding the default implementation.
We should consider marking the sync versions on the interface as
[Obsolete]and point out to the new async versions.Alternatively we could consider doing a breaking change here since it's very early in the release and we have time to advertise it.
The goal is to minimize the problems due to incompatible libraries for people updating to 6.0.