Bypass cleanup send guard in BaseRendererControl - #399
Conversation
There was a problem hiding this comment.
🟡 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 = truefrom the start ofDisposeAsync()to thefinallyblock afterTrySendCleanupAsync()completes. - Keep
_shouldReevaluateRuntimeenabled 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.
| finally | ||
| { | ||
| _shouldReevaluateRuntime = false; | ||
| disposedValue = true; | ||
| _objRef?.Dispose(); |
There was a problem hiding this comment.
Ah, might have a valid point there.
There was a problem hiding this comment.
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.
|
@MayaKirova The tests in #394 do have some overlap, at least There should be at least a small clean test like DisposeAsync_SendsCleanupMessage (Render and ready a component, @copilot would you be so kind :) |
Co-authored-by: damyanpetev <3198469+damyanpetev@users.noreply.github.com>
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>
Restore early disposal state and bypass cleanup send guard in BaseRendererControl
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):
Component(s) / Area(s) Affected:
How Has This Been Tested?
Test Configuration:
Screenshots / Recordings
Checklist:
README.MDCHANGELOG.MDupdates for newly added functionalityCloses #