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

Support IAsyncDisposable #9960

Closed
chucker opened this issue May 3, 2019 · 8 comments
Closed

Support IAsyncDisposable #9960

chucker opened this issue May 3, 2019 · 8 comments
Assignees
Labels
area-blazor Includes: Blazor, Razor Components Components Big Rock This issue tracks a big effort which can span multiple issues Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one
Milestone

Comments

@chucker
Copy link

chucker commented May 3, 2019

Is your feature request related to a problem? Please describe.

I implement IDisposable in some components to interop with JS, to clean up resources on the JS end. Blazor calls Dispose() on my component as appropriate.

However, this requires using IJSInProcessRuntime to get synchronous JS invocations.

Describe the solution you'd like

If the component implements IAsyncDisposable, Blazor should call (await) DisposeAsync.

@pranavkm pranavkm added area-blazor Includes: Blazor, Razor Components area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels May 4, 2019
@SteveSandersonMS
Copy link
Member

Agreed - thanks for the suggestion!

@SteveSandersonMS SteveSandersonMS added this to the 3.0.0-preview7 milestone May 7, 2019
@SteveSandersonMS SteveSandersonMS changed the title [Components] Support IAsyncDisposable Support IAsyncDisposable May 7, 2019
@mkArtakMSFT mkArtakMSFT removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels May 9, 2019
@mkArtakMSFT mkArtakMSFT modified the milestones: 3.0.0-preview7, 3.1.0 May 23, 2019
@mkArtakMSFT mkArtakMSFT modified the milestones: 3.1.0, 5.0.0 Aug 12, 2019
@mkArtakMSFT mkArtakMSFT added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label Aug 12, 2019
@rynowak
Copy link
Member

rynowak commented Aug 14, 2019

When we add support for IAsyncDisposable for components, we need to also enable this in OwningComponentBase

@ryanbrandenburg
Copy link
Contributor

@rynowak I was trying to figure out what was necessary to make this work and it seems like this would have to by made to return a task, which would bubble up to the point where we have to change public API's. Is that expected or do we have a way to call this asynchronously without changing a ton of API surface?

@rynowak
Copy link
Member

rynowak commented Nov 6, 2019

It depends which public APIs there are. I think the renderer is declared as unstable.

@ryanbrandenburg ryanbrandenburg self-assigned this Nov 6, 2019
@SteveSandersonMS
Copy link
Member

I suspect we wouldn't want to change the incoming code paths to become async, since they are right in the middle of rendering which aims to be as synchronous as possible.

What we might do instead is have something like a List<Task> PendingAsyncDisposeTasks on the Renderer, and add the returned tasks to it. Then callers could optionally do an await Task.WhenAll(PendingAsyncDisposeTasks) after (for example) raising an event inside the renderer, so they can propagate any exceptions. I expect we can find a sensible place to put such an await for both server-side and client-side cases. Nah, that sounds too difficult.

It might be sufficient to chain onto the returned disposal promises either using ContinueWith or more likely using await inside a fire-and-forget async method, and for any exceptions, pass it through to Dispatcher.OnUnhandledException.

@ryanbrandenburg
Copy link
Contributor

Yeah, the real problem seems to start with RenderHandle.Render needing to turn into RenderHandle.RenderAsync After that all the instances of IComponent which call RenderHandle.Render inside their Attach method are in trouble, and I suspect that's a deal-breaker.

Steve's suggestion sounds reasonable, though I'd have to poke around a bit deeper to understand the details. The other idea I've heard is @NTaylorMullen suggested that rather than modify existing API surface we could add new Async APIs and have both. Given that it's apparently a goal/requirement to keep Rendering sync I imagine that's not the preferred solution?

@javiercn
Copy link
Member

javiercn commented Jul 9, 2020

Async disposable support for Blazor

Currently we only support disposing components synchronously. For disposable components there are several design decissions that we've made along the way:

  • Within a render batch we process the disposed child components for a given component as a bundle, and produce a list of errors each component we process.
  • We don't however, stop the render batch from running. Instead we simply notify the error and let the renderer deal with it within its handle error implementation.
    • RemoteRenderer will terminate the circuit.
    • WebAssemblyRenderer will log the error to the console.
  • A component being disposed can trigger a render somewhere else due to some notification during disposal, for example.

All these behaviors happen synchronously, so that means that a render batch only finishes rendering after all the components have been disposed and there are no more renders to process.

Supporting async disposal brings in several questions:

  • Do we wait for disposed child components within a given component render to finish disposing before continuing the work?
  • Do we wait to send UI updates until all components have successfully been disposed asynchronously?
  • Do we trigger new render batches after each component has completed disposing or do we wait until all of the components disposed in a previous batch have finished disposing?

Overview of the design

  • We want disposal to behave in a similar way as event handlers and lifecycle methods.
  • If as part of disposing an async disposable component a render is triggered within the syncrhonous piece, that render belongs in the same render batch being currently processed.
  • Otherwise, it belongs in a separate render batch.
  • Synchronous exceptions within async disposables are reported right away.
  • Asynchronous exceptions are reported once the task completes.
  • We report all the asynchronous disposal errors for a single render batch at the same time.
    • This simplifies processing, but we can also group them by component instead.
    • That said, synchronous errors will be reported before asynchronous errors are so as to not wait for them to complete.

@javiercn
Copy link
Member

Done in 763ccb2

@javiercn javiercn added Done This issue has been fixed and removed Working labels Jul 16, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Aug 15, 2020
@danroth27 danroth27 modified the milestones: 5.0.0-preview8, 5.0.0-rc1 Sep 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components Components Big Rock This issue tracks a big effort which can span multiple issues Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one
Projects
None yet
Development

No branches or pull requests

8 participants