Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Select: Fix memory leak caused by key interceptor (#4060) #4076

Merged
merged 1 commit into from
May 12, 2022

Conversation

duckblaster
Copy link
Contributor

@duckblaster duckblaster commented Mar 2, 2022

This fixes #4060, the problem is caused by the transient KeyInterceptor holding a reference to the component via the KeyUp/Down events.
Since it is not disposed until the scope is also disposed, and the scope is disposed when the Blazor connection is, rather than the page, it hangs around and can't be collected.

Description

How Has This Been Tested?

Tested manually, using https://github.com/lachlanwgordon/MudBlazorMemoryLeakRepro and triggering a GC to confirm.
I don't know how to test this automatically, so have not added a unit test.

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.

@JonBunator JonBunator added bug Something does not work as intended/expected needs review labels Mar 13, 2022
@codecov
Copy link

codecov bot commented Mar 13, 2022

Codecov Report

Merging #4076 (0b049a0) into dev (3616f97) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev    #4076      +/-   ##
==========================================
+ Coverage   91.26%   91.28%   +0.01%     
==========================================
  Files         361      361              
  Lines       12465    12478      +13     
==========================================
+ Hits        11376    11390      +14     
+ Misses       1089     1088       -1     
Impacted Files Coverage Δ
src/MudBlazor/Components/Tabs/MudTabPanel.razor 100.00% <ø> (ø)
src/MudBlazor/Components/Select/MudSelect.razor.cs 96.75% <100.00%> (+0.03%) ⬆️
src/MudBlazor/Components/Tabs/MudTabs.razor 100.00% <100.00%> (ø)
...udBlazor/Services/KeyInterceptor/KeyInterceptor.cs 78.78% <100.00%> (+4.59%) ⬆️

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 ed4c3e8...0b049a0. Read the comment docs.

@boukenka
Copy link
Contributor

boukenka commented Apr 21, 2022

@duckblaster Hello. Thank you for your PR. Do you think you could solve the branch conflicts?

@duckblaster
Copy link
Contributor Author

@boukenka rebased to latest, is everything good now?

@boukenka
Copy link
Contributor

boukenka commented Apr 22, 2022

@duckblaster Thank you. I took a look at the MudSelect.razor.cs and KeyInterceptor.cs files. It looks good to me.
@henon @MattChique You are the two people who contributed the KeyInterceptor.cs file. Could one of you take a look at the PR please.

@henon
Copy link
Collaborator

henon commented May 12, 2022

We have another PR #4544 which is addressing the same problem in a different way.

@henon henon changed the title Fix MudSelect not being Garbage Collected (#4060) Select: Fix memory leak caused by key interceptor (#4060) May 12, 2022
@henon henon merged commit 558707d into MudBlazor:dev May 12, 2022
@henon
Copy link
Collaborator

henon commented May 12, 2022

I decided to merge it anyway because we don't know when the other PR is gonna be merged. Thanks!

@henon henon added this to the 6.0.11 milestone May 12, 2022
jammerware pushed a commit to jammerware/MudBlazor that referenced this pull request Sep 20, 2022
jammerware pushed a commit to jammerware/MudBlazor that referenced this pull request Sep 20, 2022
jammerware pushed a commit to jammerware/MudBlazor that referenced this pull request Sep 20, 2022
jammerware pushed a commit to jammerware/MudBlazor that referenced this pull request Sep 20, 2022
ScarletKuro pushed a commit to ScarletKuro/MudBlazor that referenced this pull request Mar 27, 2023
ferraridavide pushed a commit to ferraridavide/MudBlazor that referenced this pull request May 30, 2023
ilovepilav pushed a commit to ilovepilav/MudBlazor that referenced this pull request Nov 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something does not work as intended/expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MudSelect with ValueChanged callback causes page to never be Garbage Collected
4 participants