MudPopover: Fixes leaks caused by concurrency between OnAfterRenderAsync and DisposeAsync #3963

diegofrata · 2022-02-15T18:16:58Z

Description

Attempts fixing #3961 by controlling the concurrency with a semaphore slim.

In certain situations, 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 and slowing down the application over time.

This PR addresses two scenarios:

Calls are made concurrently and reach Initialize first and then Detach on MudPopoverHandler
In this case, everything works as expected as the semaphore will prevent Detach from proceeding until Initialise finishes.
Call are made concurrently and reach Detach first and then Initialise
In this case, the MudPopoverHandler will be marked as being detached and Initialize will be a no-op.

It also aims to improve overall performance by replacing MudPopoverService list of handlers with a dictionary.

How Has This Been Tested?

bUnit tests handling the above cases.
Following the steps to reproduce the issue in a local build of MudBlazor.Docs.Server

Types 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)

Checklist:

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

codecov · 2022-02-15T18:26:04Z

Codecov Report

Merging #3963 (25b3193) into dev (f729d2a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##              dev    #3963   +/-   ##
=======================================
  Coverage   91.12%   91.12%           
=======================================
  Files         351      351           
  Lines       11603    11612    +9     
=======================================
+ Hits        10573    10582    +9     
  Misses       1030     1030

Impacted Files	Coverage Δ
.../MudBlazor/Components/Popover/MudPopoverService.cs	`97.05% <100.00%> (+0.33%)`	⬆️
...dBlazor/Components/TextField/MudTextField.razor.cs	`75.00% <0.00%> (+1.31%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f729d2a...25b3193. Read the comment docs.

just-the-benno · 2022-02-16T13:28:13Z

src/MudBlazor/Components/Popover/MudPopoverService.cs


-        public Guid Id { get; init; }
+        public Guid Id { get; private set; }


Why did you remove init? The Id is only set once in the constructor?

There was no code actually using the init setter and there's no reason for it to set be set outside the ctor.

And isn't that the reason to have init instead of private? To make sure this property is only set in the constructor?

To be fair, the correct solution here would be:

public Guid Id { get; }

No setter means it can only be initialized in the constructor. A private set allows the property to be set from within the class. init however is a public setter that can only be used either in the constructor OR in the initialization block, the latter being a publicly usage which we really don't want.

I will amend the PR!

just-the-benno · 2022-02-16T13:31:23Z

src/MudBlazor/Components/Popover/MudPopoverService.cs

+                _detached = true;
+
+                if (IsConnected)
+                    await _runtime.InvokeVoidAsync("mudPopover.disconnect", Id);


please wrap the call inside { }.

No worries! Done.

just-the-benno · 2022-02-16T13:37:11Z

Hi @diegofrata and thanks for your work.

For some reason, I've missed the concurrent scenario in this case. Other components and handlers do use a SemaphoreSlim so thanks for catching this one. Your analysis and solution is valid. Just a minor styling issue.

diegofrata · 2022-02-16T13:45:38Z

To be honest it's the first time I run into this behavior with Blazor. I am not entirely sure I agree with it, I don't think Dispose should ever run concurrently to OnAfterRenderAsync. I think this needs to be raised further with the Blazor devs, I need to find the appropriate channel for doing so. It is a massive gotcha.

Thanks for reviewing it!

just-the-benno · 2022-02-16T13:52:17Z

I had encountered other strange behavior regarding the execution syntax. If you want to see exciting effects, investigate the difference between awaiting the execution of an EventCallback and not.

…DisposeAsync. Add missing braces to if-condition Patch flaky test and remove setter from Id

diegofrata · 2022-02-16T14:19:34Z

@just-the-benno I have noticed the tests I wrote were flaky, there's no good way to test that the locking I introduced works correctly other than adding a small delay OR abstracting the entire semaphore/locking and then mocking it. I opted for a 50ms hit in the test suite here, as the latter would be a lot more work.

just-the-benno · 2022-02-16T14:35:20Z

I understand your perspective, plus the abstraction is more work and more code to maintain without a "functional" benefit. I think these concurrent situations are the "limit" of what can and should be tested within the scope of unit testing. I struggled my self in other components, too.

The only thing I would try (not sure if it works) is to render multiple popovers simultaneously and look at the js calls. If such a test passes, you go back with your source code at the start and see if the test is failing.

That is my approach to those problems:

Start the PR
Create the test case that is currently failing
Working to make it pass
Write other tests along with the new functionality

But that said, for this situation, in particular, It might not even be possible to provoke the error in the first place.

diegofrata · 2022-02-16T15:38:21Z

@just-the-benno the semaphore is per popover in this case, not global, so I don't think the suggestion can be applied! I am confident the test demonstrates the fix though.

For future work, I think components that rely on interop should inherit from a special base class that overrides and seals both OnAfterRenderAsync and DiposeAsync and expose virtual methods that are already wrapped with a semaphore.

Are you happy to merge as is? I am quite keen on having this released soon to improve the performance of my app, currently grinding to a halt after a number of renders (lots of tooltips!).

just-the-benno · 2022-02-16T15:49:13Z

Yes, per handler, that slipped my mind while writing the text above.

While not opposing the idea of a base class, keep in mind that there are different types of interop calls. Some are stateless, like GetBoundingRect or something, while others, like the popover, rely on synchronizing the state in both spaces, Blazor and js. So, the problem is not the interop but the state management, and maybe in the future, we will find better ways to do so.

I need to investigate why a non-related test is failing. And as soon as it is solved, we can merge it.

Fixes memory leak caused by concurrent calls of AfterRenderAsync and DisposeAsync. (MudBlazor#3963)

lonix1 · 2022-12-27T23:33:44Z

I don't know for sure if it's applicable here, but the blazor team recommends using a js MutationObserver to handle disposal in blazor-to-js interop. Why did you guys opt to use locking in this case? I ran into this problem too.

Fixes memory leak caused by concurrent calls of AfterRenderAsync and DisposeAsync. (MudBlazor#3963)

Garderoben added bug Something does not work as intended/expected needs review labels Feb 16, 2022

Garderoben requested a review from just-the-benno February 16, 2022 07:08

just-the-benno reviewed Feb 16, 2022

View reviewed changes

diegofrata force-pushed the popover-fix branch from 9ba9362 to 314b614 Compare February 16, 2022 13:39

just-the-benno approved these changes Feb 16, 2022

View reviewed changes

Fixes memory leak caused by concurrent calls of AfterRenderAsync and …

25b3193

…DisposeAsync. Add missing braces to if-condition Patch flaky test and remove setter from Id

diegofrata force-pushed the popover-fix branch from 314b614 to 25b3193 Compare February 16, 2022 14:16

Garderoben added this to the 6.0.7 milestone Feb 16, 2022

Garderoben merged commit eb4712a into MudBlazor:dev Feb 16, 2022

mckaragoz mentioned this pull request Mar 3, 2022

MudSelect don't close dropdown when you select an item (on small screens, BSS only) #4063

Closed

2 tasks

jammerware pushed a commit to jammerware/MudBlazor that referenced this pull request Sep 20, 2022

MudPopover: Fixes memory leak (MudBlazor#3963)

ee3cc50

Fixes memory leak caused by concurrent calls of AfterRenderAsync and DisposeAsync. (MudBlazor#3963)

jammerware pushed a commit to jammerware/MudBlazor that referenced this pull request Sep 20, 2022

MudPopover: Fixes memory leak (MudBlazor#3963)

1a96d90

Fixes memory leak caused by concurrent calls of AfterRenderAsync and DisposeAsync. (MudBlazor#3963)

jammerware pushed a commit to jammerware/MudBlazor that referenced this pull request Sep 20, 2022

MudPopover: Fixes memory leak (MudBlazor#3963)

2dedc2c

Fixes memory leak caused by concurrent calls of AfterRenderAsync and DisposeAsync. (MudBlazor#3963)

jammerware pushed a commit to jammerware/MudBlazor that referenced this pull request Sep 20, 2022

MudPopover: Fixes memory leak (MudBlazor#3963)

99e7a08

Fixes memory leak caused by concurrent calls of AfterRenderAsync and DisposeAsync. (MudBlazor#3963)

This was referenced Dec 27, 2022

Blazor component removed from DOM before disposal causes js interop to fail dotnet/aspnetcore#45777

Closed

Managed way to handle js disposal in blazor-to-js interop dotnet/aspnetcore#45786

Closed

3dots pushed a commit to 3dots/MudBlazor that referenced this pull request Mar 23, 2023

MudPopover: Fixes memory leak (MudBlazor#3963)

e68e1dc

Fixes memory leak caused by concurrent calls of AfterRenderAsync and DisposeAsync. (MudBlazor#3963)

ScarletKuro mentioned this pull request May 24, 2023

Large dispose time of multiple MudPopover #6498

Closed

2 tasks

ferraridavide pushed a commit to ferraridavide/MudBlazor that referenced this pull request May 30, 2023

MudPopover: Fixes memory leak (MudBlazor#3963)

25e23c1

Fixes memory leak caused by concurrent calls of AfterRenderAsync and DisposeAsync. (MudBlazor#3963)

ilovepilav pushed a commit to ilovepilav/MudBlazor that referenced this pull request Nov 25, 2023

MudPopover: Fixes memory leak (MudBlazor#3963)

0de3b90

Fixes memory leak caused by concurrent calls of AfterRenderAsync and DisposeAsync. (MudBlazor#3963)

ScarletKuro mentioned this pull request Feb 22, 2024

PopoverService: Improve performance #8210

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MudPopover: Fixes leaks caused by concurrency between OnAfterRenderAsync and DisposeAsync #3963

MudPopover: Fixes leaks caused by concurrency between OnAfterRenderAsync and DisposeAsync #3963

diegofrata commented Feb 15, 2022 •

edited

Loading

codecov bot commented Feb 15, 2022 •

edited

Loading

just-the-benno Feb 16, 2022

diegofrata Feb 16, 2022

just-the-benno Feb 16, 2022

diegofrata Feb 16, 2022

just-the-benno Feb 16, 2022

diegofrata Feb 16, 2022

just-the-benno commented Feb 16, 2022

diegofrata commented Feb 16, 2022

just-the-benno commented Feb 16, 2022

diegofrata commented Feb 16, 2022

just-the-benno commented Feb 16, 2022 •

edited

Loading

diegofrata commented Feb 16, 2022 •

edited

Loading

just-the-benno commented Feb 16, 2022

lonix1 commented Dec 27, 2022


		public Guid Id { get; init; }
		public Guid Id { get; private set; }

MudPopover: Fixes leaks caused by concurrency between OnAfterRenderAsync and DisposeAsync #3963

MudPopover: Fixes leaks caused by concurrency between OnAfterRenderAsync and DisposeAsync #3963

Conversation

diegofrata commented Feb 15, 2022 • edited Loading

Description

How Has This Been Tested?

Types of changes

Checklist:

codecov bot commented Feb 15, 2022 • edited Loading

Codecov Report

just-the-benno Feb 16, 2022

Choose a reason for hiding this comment

diegofrata Feb 16, 2022

Choose a reason for hiding this comment

just-the-benno Feb 16, 2022

Choose a reason for hiding this comment

diegofrata Feb 16, 2022

Choose a reason for hiding this comment

just-the-benno Feb 16, 2022

Choose a reason for hiding this comment

diegofrata Feb 16, 2022

Choose a reason for hiding this comment

just-the-benno commented Feb 16, 2022

diegofrata commented Feb 16, 2022

just-the-benno commented Feb 16, 2022

diegofrata commented Feb 16, 2022

just-the-benno commented Feb 16, 2022 • edited Loading

diegofrata commented Feb 16, 2022 • edited Loading

just-the-benno commented Feb 16, 2022

lonix1 commented Dec 27, 2022

diegofrata commented Feb 15, 2022 •

edited

Loading

codecov bot commented Feb 15, 2022 •

edited

Loading

just-the-benno commented Feb 16, 2022 •

edited

Loading

diegofrata commented Feb 16, 2022 •

edited

Loading