Skip to content

PopoverService: Remove locking#9938

Merged
ScarletKuro merged 2 commits intoMudBlazor:devfrom
ScarletKuro:popover_freelock
Oct 8, 2024
Merged

PopoverService: Remove locking#9938
ScarletKuro merged 2 commits intoMudBlazor:devfrom
ScarletKuro:popover_freelock

Conversation

@ScarletKuro
Copy link
Member

@ScarletKuro ScarletKuro commented Oct 8, 2024

Description

We received this complaint: #8325 (comment). Even though we haven't received any actual feedback from them yet, I decided to remove the locking mechanism beforehand.

Historically, the locking mechanism was added in this PR: #3963:

Blazor will call DisposeAsync while OnAfterRenderAsync is still being executed. Apparently, this is by design, and when it happens to a MudPopover component, it leads to the handler being detached first and then initialized, leaving the JS objects in the browser.

I don’t think this is necessary anymore because:

  1. We have sufficient checks to see if the service has been disposed.
  2. We now have a CancellationTokenSource that is triggered on disposal, which means that technically all currently running (or future) JS calls should be aborted. This was added in PopoverService: Attemp to fix SemaphoreFullException #8325 to prevent SemaphoreFullException, but eventually it should make the locking non required.

How Has This Been Tested?

I think only time can show.
The problem can occur only on BSS under specific conditions that it hard to reproduce on localhost.

Type of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation (fix or improvement to the website or code docs)

Checklist

  • The PR is submitted to the correct branch (dev).
  • My code follows the code style of this project.
  • I've added relevant tests.

@github-actions github-actions bot added bug Unexpected behavior or functionality not working as intended PR: needs review labels Oct 8, 2024
@ScarletKuro ScarletKuro merged commit b043181 into MudBlazor:dev Oct 8, 2024
@codecov
Copy link

codecov bot commented Oct 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.65%. Comparing base (28bc599) to head (e33283d).
Report is 522 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #9938      +/-   ##
==========================================
+ Coverage   89.82%   90.65%   +0.82%     
==========================================
  Files         412      409       -3     
  Lines       11878    12639     +761     
  Branches     2364     2466     +102     
==========================================
+ Hits        10670    11458     +788     
+ Misses        681      616      -65     
- Partials      527      565      +38     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ScarletKuro ScarletKuro deleted the popover_freelock branch October 9, 2024 09:24
LLauter pushed a commit to cannellamedia/MudBlazor that referenced this pull request Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Unexpected behavior or functionality not working as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant