-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Remove await Task.FromResult()
and unnecessary async modifiers
#16535
base: v16/dev
Are you sure you want to change the base?
Conversation
return Ok(viewModels.Reverse()); | ||
} | ||
|
||
private async IAsyncEnumerable<NamedEntityTreeItemResponseModel> MapTreeItemViewModelsAsync(IEnumerable<IDictionaryItem> dictionaryItems) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By using a method that returns IAsyncEnumerable
, we're able to asynchronously iterate over the items by using await foreach (var item in MapTreeItemViewModelsAsync(...))
, await MapTreeItemViewModelsAsync(...).ToArrayAsync()
or any other To...Async()
methods...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even better would be to actually return the IAsyncEnumerable
as model (when possible), because that will avoid loading all items into memory and JSON serialize each item one by one.
public override async Task<IEnumerable<HealthCheckStatus>> GetStatus() | ||
=> [ | ||
await CheckIfCurrentSchemeIsHttps(), | ||
await CheckHttpsConfigurationSetting(), | ||
await CheckForValidCertificate() | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This previously started all tasks at the same time, but uses the ILocalizedTextService
to lookup translations (and any Umbraco specific code, especially services, can create scopes). This could be rewritten to do the actual work concurrently and get nicely formatted health check status messages afterwards, but that might be something for later/another PR.
# Conflicts: # src/Umbraco.Cms.Api.Management/Controllers/Document/Item/SearchDocumentItemController.cs # src/Umbraco.Cms.Api.Management/Controllers/HealthCheck/Group/CheckHealthCheckGroupController.cs # src/Umbraco.Cms.Api.Management/Controllers/Media/Item/SearchMediaItemController.cs # src/Umbraco.Cms.Api.Management/Controllers/Member/Item/SearchMemberItemController.cs # src/Umbraco.Cms.Api.Management/Controllers/PublishedCache/RebuildPublishedCacheController.cs # src/Umbraco.Cms.Api.Management/Controllers/PublishedCache/StatusPublishedCacheController.cs # src/Umbraco.Cms.Api.Management/Factories/DocumentNotificationPresentationFactory.cs # src/Umbraco.Cms.Api.Management/Factories/UserPresentationFactory.cs # src/Umbraco.Cms.Persistence.EFCore/Locking/SqliteEFCoreDistributedLockingMechanism.cs # src/Umbraco.Core/HealthChecks/Checks/Security/BaseHttpHeaderCheck.cs # src/Umbraco.Core/HealthChecks/Checks/Security/ExcessiveHeadersCheck.cs # src/Umbraco.Core/HealthChecks/Checks/Security/HttpsCheck.cs # src/Umbraco.Core/Services/ContentEditingService.cs # src/Umbraco.Core/Services/UserGroupPermissionService.cs # src/Umbraco.Infrastructure/Services/MemberEditingService.cs # tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/BackgroundJobs/Jobs/HealthCheckNotifierJobTests.cs # tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/HealthChecks/HealthCheckResultsTests.cs
# Conflicts: # src/Umbraco.Core/Services/ContentEditingServiceBase.cs # src/Umbraco.Core/Services/ContentEditingServiceWithSortingBase.cs # src/Umbraco.Core/Services/MediaEditingService.cs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have reviewed and smoke tested, and seems good, but it's a lot of changes. I don't think it's particularly risky but minded to retarget to v16/dev
and merge in there, just as there's no rush, and so we have it in a version with a longer testing period.
Prerequisites
Description
While working on another fix, I noticed we have quite a few occurrences of
await Task.FromResult()
(as mentioned in #16430 (comment)). This indicates a real anti-pattern, as you would only needTask.FromResult()
when you don't need to useawait
in an async method (if all work is synchronous, but your method signature is still async and returns an awaitable task). And if all work in the method is synchronous, you don't need to mark it asasync
, as that will cause the C# compiler to unnecessarily generate a state machine, see: https://learn.microsoft.com/en-us/dotnet/csharp/asynchronous-programming/async-scenarios#important-info-and-advice.Removing the
async
keyword from a method doesn't change the signature, so all those changes in this PR will only cause the compiler to emit less code (which results is smaller assemblies and faster execution).Another (Umbraco specific) anti-pattern is to use
Task.WhenAll()
when doing work that creates Umbraco scopes, as the created scopes need to be disposed in the correct/reverse order they were created or otherwise suppress the execution scope (so new root scopes are created instead):Umbraco-CMS/src/Umbraco.Infrastructure/Scoping/Scope.cs
Line 367 in d1aac39
I've updated most of these cases, although some will now not execute the tasks in parallel anymore. But it's better to not risk the chance of scopes being disposed out of order and throwing exception or getting deadlocks, and take the penalty of a slight decrease in performance instead.
Testing will mostly be validating the code changes and ensuring the build and tests run as expected, as there's really not a lot of functional changes in this PR 🤓