Skip to content

Components: Use IAsyncDisposable#10037

Merged
ScarletKuro merged 7 commits intoMudBlazor:devfrom
ScarletKuro:dispose
Oct 16, 2024
Merged

Components: Use IAsyncDisposable#10037
ScarletKuro merged 7 commits intoMudBlazor:devfrom
ScarletKuro:dispose

Conversation

@ScarletKuro
Copy link
Member

@ScarletKuro ScarletKuro commented Oct 15, 2024

Description

We use async code almost everywhere, so it’s necessary.

Answering some questions beforehand:

Q: Why is it ValueTask DisposeAsyncCore() instead of ValueTask DisposeAsync(bool disposing)?
A: This follows Microsoft’s recommendation. See: Explore DisposeAsync and DisposeAsyncCore methods.

Q: Why are we not implementing both IDisposable and IAsyncDisposable? Isn’t it Microsoft’s recommendation, which comes with a big red warning?
A: This applies only when the consumer needs to manage disposal. However, services and components are managed either by DI or the Blazor lifecycle, which correctly disposes of resources. Implementing both interfaces could cause more harm and confusion, in my opinion.

Q: Why GC.SuppressFinalize(this) is not everywhere?
A: Not needed if the class is sealed and there is no finilizer in the class and consumer can't add his own since it's sealed.

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 enhancement Adds a new feature or enhances existing functionality (not fixing a defect) in the main library PR: needs review labels Oct 15, 2024
@ScarletKuro ScarletKuro added breaking change This change will require consumer code updates and removed enhancement Adds a new feature or enhances existing functionality (not fixing a defect) in the main library labels Oct 15, 2024
@codecov
Copy link

codecov bot commented Oct 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.05%. Comparing base (4429bc1) to head (316993e).
Report is 2 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev   #10037      +/-   ##
==========================================
- Coverage   91.06%   91.05%   -0.02%     
==========================================
  Files         410      410              
  Lines       12514    12451      -63     
  Branches     2446     2427      -19     
==========================================
- Hits        11396    11337      -59     
+ Misses        570      566       -4     
  Partials      548      548              

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

Copy link
Contributor

@jperson2000 jperson2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvement, @ScarletKuro! I see why you chose not to implement IAsyncDisposable in MudComponentBase; a majority of components don't need to dispose anything, so it makes sense to do them case-by-case.

I see some additional components which qualify for IAsyncDisposable when I do a Find All References on IDisposable:

image

@ScarletKuro
Copy link
Member Author

ScarletKuro commented Oct 15, 2024

I see some additional components which qualify for IAsyncDisposable when I do a Find All References on IDisposable

It's very individual. If the components doesn't have async disposable instances, you can stick with IDisposable to avoid overhead (negligible, but still). This is mainly a follow-up PR for: #9956.

The UnregisterAsync method returns a Task, which is preferable to await. Not doing so can lead to quite annoying race conditions that happened to drawer when it wasn't awaiting the IBrowserViewportService.UnsubscribeAsync when we made lock free logic.
Since the above PR was done for v7, we couldn't simply change it to IAsyncDisposable, as most components that use it inherit from MudFormComponent, which has a protected virtual void Dispose method. Changing its signature would be a breaking change since end customers can inherit this base class as well. Now we can do it in v8.

But thanks for the list. I'm sure that some of them have async disposable instances and are using CatchAndLog instead of awaiting.

@ScarletKuro ScarletKuro added the v8 label Oct 15, 2024
@ScarletKuro
Copy link
Member Author

Added to the v8 migration guide at #9953

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change This change will require consumer code updates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants