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

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

Open
wants to merge 6 commits into
base: contrib
Choose a base branch
from

Conversation

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant