Skip to content

Bypass cleanup send guard in BaseRendererControl - #399

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

Bypass cleanup send guard in BaseRendererControl#399
MayaKirova wants to merge 7 commits into
masterfrom
mkirova/fix-disposal

Conversation

@MayaKirova

Copy link
Copy Markdown
Contributor

Description

Fix regression where the tag was set before the last cleanup message was sent.
There are tests for the disposal here: #394

Motivation / Context

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:

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 #

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.

🟡 Changes recommended

Delaying disposedValue until after awaiting cleanup introduces a window where other code can enqueue messages that may flush after cleanup, undermining the “cleanup is last message” guarantee.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR adjusts BaseRendererControl.DisposeAsync() so the internal disposed flag is set after attempting to send the final cleanup renderer message, addressing a regression where the flag prevented that last cleanup send.

Changes:

  • Move disposedValue = true from the start of DisposeAsync() to the finally block after TrySendCleanupAsync() completes.
  • Keep _shouldReevaluateRuntime enabled during the cleanup send attempt, then reset it during teardown.
File summaries
File Description
src/componentsBase/BaseRendererControl.cs Reorders disposal flagging to allow the final cleanup message to be sent before marking the control disposed.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Lite

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

Comment on lines 3151 to 3155
finally
{
_shouldReevaluateRuntime = false;
disposedValue = true;
_objRef?.Dispose();

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.

Ah, might have a valid point there.

@damyanpetev damyanpetev Sep 8, 2026

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.

Yeah, inverting the order might not suffice in all cases, so the direct SendJsonImmediate bypass might be the way to go, queued an agent to do that instead and we'll see how that stacks up.
Edit: Leaving the actual thread synchronizations to #394 explicitly, if the disposedValue set remains early, the changes there will resolve.

@damyanpetev

Copy link
Copy Markdown
Member

@MayaKirova The tests in #394 do have some overlap, at least DisposeAsync_SendsNothingAfterCleanup but its concurrency setup makes failures harder to diagnose and it's goal is not to test the straight-forward scenario.

There should be at least a small clean test like DisposeAsync_SendsCleanupMessage (Render and ready a component,
call DisposeAsync, assert exactly 1 igSendMessage and "type": "cleanup") and DisposeAsync_CalledTwice_IsIdempotent should assert cleanup was sent exactly once, not merely that the second call did not throw.

@copilot would you be so kind :)

Co-authored-by: damyanpetev <3198469+damyanpetev@users.noreply.github.com>

Copilot AI commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

@MayaKirova The tests in #394 do have some overlap, at least DisposeAsync_SendsNothingAfterCleanup but its concurrency setup makes failures harder to diagnose and it's goal is not to test the straig...

Added focused cleanup-delivery coverage and strengthened idempotency to assert cleanup is sent exactly once in aa22135.

Co-authored-by: damyanpetev <3198469+damyanpetev@users.noreply.github.com>
@damyanpetev
damyanpetev removed this pull request from stack #401 September 9, 2026 13:51
@MayaKirova MayaKirova changed the title fix(disposal): Move flag after last cleanup message is sent. Bypass cleanup send guard in BaseRendererControl Sep 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants