Skip to content

Add better disposal handling after sending cleanup message to client. - #335

Open
MayaKirova wants to merge 7 commits into
masterfrom
mkirova/fix-334
Open

Add better disposal handling after sending cleanup message to client.#335
MayaKirova wants to merge 7 commits into
masterfrom
mkirova/fix-334

Conversation

@MayaKirova

Copy link
Copy Markdown
Contributor

Description

Make BaseRendererControl disposal deterministic and safe by implementing IAsyncDisposable alongside the existing IDisposable, and stop discarding the Task returned by the interop cleanup send.

Motivation / Context

Before this change, BaseRendererControl.Dispose(bool) called an async Task SendMessageImmediate(...) synchronously and discarded the returned Task, and the type implemented only IDisposable

Blazor’s guidance for components that perform asynchronous cleanup is to implement IAsyncDisposable: https://learn.microsoft.com/en-us/aspnet/core/blazor/components/component-disposal?view=aspnetcore-10.0&source=recommendations#asynchronous-iasyncdisposable

Type of Change (check all that apply):

  • Bug fix
  • New functionality
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactoring (no functional changes)
  • Documentation
  • Demos
  • CI/CD
  • Tests
  • Changelog

Component(s) / Area(s) Affected:

All

How Has This Been Tested?

  • Unit tests
  • Manual testing
  • Automated e2e tests

Test Configuration:

  • .NET version:
  • Hosting model:
  • Browser(s):
  • OS:

Screenshots / Recordings

Checklist:

  • All relevant tags have been applied to this PR
  • This PR includes unit tests covering all the new code
  • This PR includes API docs for newly added methods/properties
  • This PR includes general feature table updates in the root README.MD
  • This PR includes CHANGELOG.MD updates for newly added functionality
  • This PR contains breaking changes
  • This PR includes behavioral changes and the feature specification has been updated with them
  • Accessibility (ARIA, keyboard navigation, focus management) has been verified

Closes #334

Copilot AI lite review requested due to automatic review settings August 18, 2026 13:09
Comment thread src/componentsBase/BaseRendererControl.cs Fixed
Comment thread src/componentsBase/BaseRendererControl.cs Fixed
Comment thread src/componentsBase/BaseRendererControl.cs Fixed
Comment thread src/componentsBase/BaseRendererControl.cs Fixed
Comment thread src/componentsBase/BaseRendererControl.cs Fixed

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates BaseRendererControl (the core base type for Ignite UI Blazor wrapper components) to make JS-bound cleanup more deterministic by adding an async disposal path and awaiting the interop cleanup message when Blazor chooses async disposal.

Changes:

  • Implement IAsyncDisposable on BaseRendererControl and add DisposeAsync() to allow awaited cleanup.
  • Replace the synchronous cleanup sender with an async TrySendCleanupAsync() that awaits SendMessageImmediate(...) and swallows common teardown exceptions.
  • Adjust Dispose(bool) to trigger cleanup via the new async helper (fire-and-forget) while continuing to support the existing IDisposable pattern.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/componentsBase/BaseRendererControl.cs Outdated
Comment thread src/componentsBase/BaseRendererControl.cs Outdated
Comment thread src/componentsBase/BaseRendererControl.cs Dismissed
@MayaKirova
MayaKirova requested a review from damyanpetev August 20, 2026 13:24
@damyanpetev

Copy link
Copy Markdown
Member

Um, was going to ask, but found it in the docs right away:

Components shouldn't need to implement IDisposable and IAsyncDisposable simultaneously. If both are implemented, the framework only executes the asynchronous overload.

https://learn.microsoft.com/en-us/aspnet/core/blazor/components/component-disposal?view=aspnetcore-10.0

So wouldn't the change be more of a conversion IDisposable -> IAsyncDisposable?

@dkamburov dkamburov added the ✅ status: verified Applies to PRs that have passed manual verification label Aug 20, 2026
dkamburov
dkamburov previously approved these changes Aug 20, 2026
@dkamburov
dkamburov self-requested a review August 20, 2026 14:21

@damyanpetev damyanpetev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, LGTM, though I hope we can replace the cleanup message with a module dispose per modern practices soon enough.
The only bit of concern I have is if we should be marking this as a breaking change since the components no longer implement IDisposable and neither version of keeping it very clean and would rather not. Should be improbable manual use, since both the ASP.NET framework and test (bUnit) require users to do manual disposal; Still on the table tho.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

✅ status: verified Applies to PRs that have passed manual verification

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Better Disposal handling (Disposal across the JavaScript boundary)

4 participants