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.

Sign up for GitHub

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

Jump to bottom

Use dotnet ForEachAsync implementation #15936

Merged
merged 2 commits into from
May 1, 2024
Merged

Use dotnet ForEachAsync implementation #15936

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
03d531a
Use dotnet ForEachAsync implementation
sebastienros May 1, 2024
d6596fc
Merge branch 'main' into sebros/concurrent
MikeAlhayek May 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 0 additions & 29 deletions src/OrchardCore/OrchardCore.Abstractions/Modules/Extensions/EnumerableExtensions.cs
View file
Open in desktop

This file was deleted.

6 changes: 2 additions & 4 deletions src/OrchardCore/OrchardCore/Extensions/ExtensionManager.cs
View file
Open in desktop
Piedone marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -298,11 +298,11 @@ private async Task EnsureInitializedAsync()
var loadedExtensions = new ConcurrentDictionary<string, ExtensionEntry>();

// Load all extensions in parallel
await modules.ForEachAsync((module) =>
Parallel.ForEach(modules, (module, cancellationToken) =>
Copy link
Member

Choose a reason for hiding this comment

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

Why not use Parallel.ForEachAsync() here too?

Copy link
Member

Choose a reason for hiding this comment

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

the delegate is synchronous. No need for asynchronous

Copy link
Member

Choose a reason for hiding this comment

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

EnsureInitializedAsync() is though so it can make use of async.

Copy link
Member

Choose a reason for hiding this comment

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

but that is done before the parallel call. The parallel call does not need async

Copy link
Member

Choose a reason for hiding this comment

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

Parallel.ForEach() blocks the calling thread, since it's synchronous. So, that thread will just wait, like synchronous IO does. Parallel.ForEachAsync() being async, the calling thread won't be blocked (and can thus be used to e.g. serve as one of the threads used for the loop).

Copy link
Member Author

Choose a reason for hiding this comment

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

the calling thread won't be blocked (and can thus be used to e.g. serve as one of the threads used for the loop)

But it won't because there is no async code in the parallel lambdas.

Copy link
Member

Choose a reason for hiding this comment

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

What will the thread of EnsureInitializedAsync() do until Parallel.ForEach() returns?

Copy link
Member Author

Choose a reason for hiding this comment

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

The only other thing we could change is that since nothing is asynchronous we can remove the async pattern on this method. And replace the semaphore with a lock. To remove any assumption that this is actually async.

{
if (!module.ModuleInfo.Exists)
{
return Task.CompletedTask;
return;
MikeAlhayek marked this conversation as resolved.
Show resolved Hide resolved
}

var manifestInfo = new ManifestInfo(module.ModuleInfo);
Expand All @@ -319,8 +319,6 @@ await modules.ForEachAsync((module) =>
};

loadedExtensions.TryAdd(module.Name, entry);

return Task.CompletedTask;
});

var loadedFeatures = new Dictionary<string, FeatureEntry>();
Expand Down
4 changes: 2 additions & 2 deletions src/OrchardCore/OrchardCore/Modules/ModularBackgroundService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ protected override async Task ExecuteAsync(CancellationToken stoppingToken)

private async Task RunAsync(IEnumerable<(string Tenant, long UtcTicks)> runningShells, CancellationToken stoppingToken)
{
await GetShellsToRun(runningShells).ForEachAsync(async tenant =>
await Parallel.ForEachAsync(GetShellsToRun(runningShells), async (tenant, cancellationToken) =>
MikeAlhayek marked this conversation as resolved.
Show resolved Hide resolved
{
// Check if the shell is still registered and running.
if (!_shellHost.TryGetShellContext(tenant, out var shell) || !shell.Settings.IsRunning())
Expand Down Expand Up @@ -238,7 +238,7 @@ private async Task UpdateAsync(
{
var referenceTime = DateTime.UtcNow;

await GetShellsToUpdate(previousShells, runningShells).ForEachAsync(async tenant =>
await Parallel.ForEachAsync(GetShellsToUpdate(previousShells, runningShells), async (tenant, cancellationToken) =>
MikeAlhayek marked this conversation as resolved.
Show resolved Hide resolved
{
if (stoppingToken.IsCancellationRequested)
{
Expand Down
Loading