Use dotnet ForEachAsync implementation #15936
Conversation
WalkthroughThe recent changes in the OrchardCore project focus on improving the performance by adopting parallel processing techniques. This includes modifications in Changes
Assessment against linked issues
The assessment reveals that while the code changes enhance performance through parallel processing, they do not directly address the SQL transaction issue mentioned in the linked issue #15794. Further investigation or adjustments may be required to specifically tackle the SQL exception problem during setup. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
@MikeAlhayek can you try this branch? It's fixing the issue for me. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@sebastienros this is a good change. But it did not fix the issue in reference. |
|
Not a nice rabbit. |
|
I did not think it would fix the issue here because the only thing that could impact the setup process is the change in the ExtensionManager. but event that, it should not impact it since none of the calls in that parallel logic use database call. I tried to explain what I think is happening in the comment here #15794 (comment) |
| @@ -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) => | |||
There was a problem hiding this comment.
Why not use Parallel.ForEachAsync() here too?
There was a problem hiding this comment.
the delegate is synchronous. No need for asynchronous
There was a problem hiding this comment.
EnsureInitializedAsync() is though so it can make use of async.
There was a problem hiding this comment.
but that is done before the parallel call. The parallel call does not need async
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
What will the thread of EnsureInitializedAsync() do until Parallel.ForEach() returns?
There was a problem hiding this comment.
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.
It does not fix the issue for me. Strange. Let me try cleaning up my environment and test it out again. |
|
FYI I tried a repro of both issues with a local SQL Server. |
|
@sebastienros @Piedone I just tried it again and it fails every time. I am connecting to a remote service which may be why I am getting the issue since there is higher latency than when running on a local instance. I basically added a new site on Azure SQL Database and tried to setup the tenant using Blog recipe and it fails.
|
|
It was failing every time for me before this change, then worked twice in a row, maybe I was lucky? Will continue the investigations on my side. We can still merge it since it's less custom code. |
Did you try to write to a remote SQL server instead of a local instance? Just so you have higher latency |
|
Oh and don't step through the code. remove all break points and let the code execute naturally otherwise you'll avoid the concurrency |
|
With Azure SQL the setup fails for me as well, sadly, though differently (but MARS also hints at parallel queries): BTW neither previously nor now did I have breakpoints or anything that would inhibit the normal code flow, just running with Ctrl+F5. |
Yes I think the latency is the key here. If you connect locally "very low latency" and it would work. But when you connect to a remote Azure SQL Server which has higher latency, you encounter the issue. Try to remove |
we should merge this as it is a good change. But please remove the referenced 2 issues since it does not really fix them. |
|
@sebastienros here are the last few trace logs from my logfile just before we countered the issue "maybe the queries could give you more clues on what could be updating" |
|
Without |
|
@sebastienros why did you merge? |
|
Got an approval, and your comment is not correct. |
|
You asked for my review too, and we were in a conversation, what I was quite clear about I'd like to finish before the merge. So I feel sidestepped that you merged it despite that. It's no problem if we disagree, but if I put an effort into reviewing your PR, as you asked, then I'd expect you to engage with me as well, even if I ultimately prove to be wrong. I'm happy to learn. The goal is to have good code, what also comes from discussions like this. If not by changes to the code in this PR, then the reviewer learning something and doing it better in the next one. So, can you then please tell what will the thread of |
|
Sorry, I had not seen this part:
With both solutions the caller thread will block until the parallel operation is done because the body in the parallel operation is not async, it will never yield. |
|
Why does it block with var tasks = new List<Task>();
foreach (var module in modules)
{
tasks.Add(Task.Run(...));
}
await Task.WhenAll(tasks);So, while the body lambda is executed with a given |
|
I don't think it's using Task.WhenAll at all. ThreadPool + TaskCompletionSource to detect all threads are done. Where is the documentation about ForEachAsync that would tell if it should always be used even when there is no async body? |
|
It doesn't use I'm not saying |
Co-authored-by: Mike Alhayek <[email protected]>
Summary by CodeRabbit