Skip to content

Remove await Task.FromResult() and unnecessary async modifiers#16535

Merged
AndyButland merged 8 commits intov16/devfrom
v14/feature/remove-await-task
Mar 3, 2025
Merged

Remove await Task.FromResult() and unnecessary async modifiers#16535
AndyButland merged 8 commits intov16/devfrom
v14/feature/remove-await-task

Conversation

@ronaldbarendse
Copy link
Copy Markdown
Contributor

Prerequisites

  • I have added steps to test this contribution in the description below

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 need Task.FromResult() when you don't need to use await 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 as async, 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):

$"The {nameof(Scope)} {InstanceId} being disposed is not the Ambient {nameof(Scope)} {_scopeProvider.AmbientScope?.InstanceId.ToString() ?? "NULL"}. This typically indicates that a child {nameof(Scope)} was not disposed, or flowed to a child thread that was not awaited, or concurrent threads are accessing the same {nameof(Scope)} (Ambient context) which is not supported. If using Task.Run (or similar) as a fire and forget tasks or to run threads in parallel you must suppress execution context flow with ExecutionContext.SuppressFlow() and ExecutionContext.RestoreFlow().";

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 🤓

return Ok(viewModels.Reverse());
}

private async IAsyncEnumerable<NamedEntityTreeItemResponseModel> MapTreeItemViewModelsAsync(IEnumerable<IDictionaryItem> dictionaryItems)
Copy link
Copy Markdown
Contributor Author

@ronaldbarendse ronaldbarendse May 31, 2024

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...

Copy link
Copy Markdown
Contributor Author

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.

Comment on lines +49 to +54
public override async Task<IEnumerable<HealthCheckStatus>> GetStatus()
=> [
await CheckIfCurrentSchemeIsHttps(),
await CheckHttpsConfigurationSetting(),
await CheckForValidCertificate()
];
Copy link
Copy Markdown
Contributor Author

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
@AndyButland AndyButland changed the base branch from contrib to v15/dev February 20, 2025 09:27
# Conflicts:
#	src/Umbraco.Core/Services/ContentEditingServiceBase.cs
#	src/Umbraco.Core/Services/ContentEditingServiceWithSortingBase.cs
#	src/Umbraco.Core/Services/MediaEditingService.cs
@AndyButland AndyButland changed the base branch from v15/dev to v16/dev February 21, 2025 09:01
Copy link
Copy Markdown
Contributor

@AndyButland AndyButland left a 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.

@AndyButland AndyButland merged commit 561d871 into v16/dev Mar 3, 2025
20 of 21 checks passed
@AndyButland AndyButland deleted the v14/feature/remove-await-task branch March 3, 2025 05:47
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.

3 participants