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

DataGrid: Change several public methods from async void to async Task and add Async postfix (Breaking Change) #6614

Merged
merged 1 commit into from
Apr 10, 2023

Conversation

ScarletKuro
Copy link
Member

@ScarletKuro ScarletKuro commented Apr 10, 2023

Description

This update addresses the issue of using async void in lambdas, which should be avoided whenever possible.

Previously, the design was as follows:

SetSelectedItem = async (x) => await dataGrid.SetSelectedItemAsync(x, item),
StartEditingItem = async () => await dataGrid.SetEditingItemAsync(item),
CancelEditingItem = async () => await dataGrid.CancelEditingItemAsync(),
ToggleHierarchyVisibilityForItem = async () => await dataGrid.ToggleHierarchyVisibilityAsync(item),

the SetSelectedItem, StartEditingItem, and other similar methods were defined as Actions instead of Func<Task>. This update changes the signature of these methods to return a Task, eliminating the use of async void in the lambdas.

Yes, this is breaking changed, but the sooner we fix this then better.

How Has This Been Tested?

Unit tests

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
Copy link

codecov bot commented Apr 10, 2023

Codecov Report

Patch coverage: 70.37% and project coverage change: -0.01 ⚠️

Comparison is base (a499a1a) 90.63% compared to head (7b6839e) 90.62%.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #6614      +/-   ##
==========================================
- Coverage   90.63%   90.62%   -0.01%     
==========================================
  Files         399      399              
  Lines       13565    13582      +17     
==========================================
+ Hits        12294    12309      +15     
- Misses       1271     1273       +2     
Impacted Files Coverage Δ
...udBlazor/Components/DataGrid/HierarchyColumn.razor 100.00% <ø> (ø)
...c/MudBlazor/Components/DataGrid/SelectColumn.razor 100.00% <ø> (ø)
src/MudBlazor/Components/DataGrid/CellContext.cs 83.33% <50.00%> (ø)
src/MudBlazor/Components/DataGrid/FilterContext.cs 71.42% <71.42%> (+16.88%) ⬆️
src/MudBlazor/Components/DataGrid/FooterContext.cs 90.00% <100.00%> (ø)
src/MudBlazor/Components/DataGrid/HeaderContext.cs 90.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ScarletKuro ScarletKuro requested a review from henon April 10, 2023 00:33
@ScarletKuro
Copy link
Member Author

For some reason I can't request review from @tjscience, going to just tag

@ScarletKuro
Copy link
Member Author

A reminder why we should avoid asyn void in methods, lambdas etc:

  1. No way to catch exceptions: When an async void method throws an exception, there is no way to catch it, because there is no return value to use in a try-catch block. This makes debugging difficult and can lead to unexpected behavior in your application.

  2. No way to await completion: async void methods do not return a Task, which means you cannot await their completion. This can cause issues when you need to coordinate the execution of multiple asynchronous operations, and can result in race conditions.

  3. Can cause thread pool exhaustion: async void methods are executed on a fire-and-forget basis, which means they can return control to the calling thread before they are completed. If you have a large number of async void methods executing simultaneously, you may run out of threads in the thread pool, which can cause your application to become unresponsive.

  4. Can lead to code smells: Because async void methods cannot be awaited, they are often used to fire off long-running background tasks without any way to check whether the task has completed successfully or not. This can lead to code smells, as it is difficult to ensure that the task is properly cleaned up if the application is shut down before it completes.

  5. Can make testing difficult: When you use async void methods, it can be difficult to test your code using unit tests, because there is no way to await their completion or capture any exceptions that may be thrown. This can make it harder to ensure that your code is working as intended, and can lead to bugs that are difficult to track down.

That's why I want this to be considered for a merge, even though it's a breaking change.

ApplyFilterAsync = x =>
{
//Use Task.CompletedTask but later ApplyFilter should return Task
HeaderCell?.ApplyFilter(x);
Copy link
Member Author

Choose a reason for hiding this comment

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

This is added for future. There is currently unresolved issue "the get serverdata is not called when using filtermode"(when serverdata and filter are used together). And there might be need to call ReloadServerData (which is async) in ApplyFilter etc.

@henon
Copy link
Collaborator

henon commented Apr 10, 2023

What do you think @tjscience ?

@henon
Copy link
Collaborator

henon commented Apr 10, 2023

I guess since this component is still marked as experimental on the docs as we speak we can just break now.

@tjscience
Copy link
Contributor

@henon, Yes, this is a good catch @ScarletKuro. Changes look good, thanks!

@henon henon changed the title DataGrid: Do not use async void in XYZActions DataGrid: Change several public methods from async void to async Task and add Async postfix (Breaking Change) Apr 10, 2023
@henon henon merged commit ea5f951 into MudBlazor:dev Apr 10, 2023
3 of 4 checks passed
@ScarletKuro ScarletKuro deleted the action_task branch May 28, 2023 15:48
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants