[OrchardCore.Modules.Cms.Alias] Add thread-safety for YesSql session queries. Fixes #16069

[ApacheTech]

Fixes #16069

Added a SemaphoreSlim to synchronise requests, using the AliasPartContentHandleHelper.QueryAliasIndexAsync method

Original Bug:

When loading multiple pages of a site quickly in multiple tabs, a InvalidOperationException exception is thrown, due to calling the IOrchardHelper.GetContentItemByAliasAsync() method.

InvalidOperationException: BeginExecuteReader requires an open and available Connection. The connection's current state is connecting.

    System.Threading.Tasks.ContinuationResultTaskFromResultTask<TAntecedentResult, TResult>.InnerInvoke()
    System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, object state)
    System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, object state)
    System.Threading.Tasks.Task.ExecuteWithThreadLocal(ref Task currentTaskSlot, Thread threadPoolThread)
    Dapper.SqlMapper.QueryAsync<T>(IDbConnection cnn, Type effectiveType, CommandDefinition command) in SqlMapper.Async.cs
    YesSql.Store.ProduceAsync<T, TState>(WorkerQueryKey key, Func<TState, Task<T>> work, TState state)
    YesSql.Services.DefaultQuery+Query<T>.FirstOrDefaultImpl()
    YesSql.Services.DefaultQuery+Query<T>.FirstOrDefaultImpl()
    AliasPartRazorHelperExtensions.GetContentItemIdByAliasAsync(IOrchardHelper orchardHelper, string alias)
    AliasPartRazorHelperExtensions.GetContentItemByAliasAsync(IOrchardHelper orchardHelper, string alias, bool latest)

[github-actions]

Thank you for submitting your first pull request, awesome! 🚀 If you haven't already, please take a moment to review our contribution guide. This guide provides helpful information to ensure your contribution aligns with our standards. A core team member will review your pull request.

If you like Orchard Core, please star our repo and join our community channels

[ApacheTech]

@dotnet-policy-service agree

[Piedone]

Synchronization is only a fix for the symptom, not the cause. These services shouldn't be called concurrently with the same ISession (~same scope). This is also true for all services in Orchard Core, so we'd need synchronization for all data access, which is not the feasible solution either.

If you can't prevent your application code from calling these services concurrently (which would be the first thing to try to address, see #16069 (comment)), then you need to add locking there, but not in Orchard Core.

[ApacheTech]

Synchronization is only a fix for the symptom, not the cause. These services shouldn't be called concurrently with the same ISession (~same scope). This is also true for all services in Orchard Core, so we'd need synchronization for all data access, which is not the feasible solution either.

If you can't prevent your application code from calling these services concurrently (which would be the first thing to try to address, see #16069 (comment)), then you need to add locking there, but not in Orchard Core.

That's what I asked in the first place, and was told to make a PR with a fix. I didn't know if it was an application level thing, an OrchardCore thing, or a YesSql thing.

[Piedone]

It's probably unclear at first, but under the issue, Sebastien asked you to try if locking fixes your problem, because then the problem is concurrent invocation. But locking is not a proper fix, just a way to uncover the cause of the problem.

[ApacheTech]

It's probably unclear at first, but under the issue, Sebastien asked you to try if locking fixes your problem, because then the problem is concurrent invocation. But locking is not a proper fix, just a way to uncover the cause of the problem.

I meant here: #16069 (comment). That was in response to me asking if this is something that should be added to OC at a lower level than the app level. If it's an even deeper problem than that, then that's ok. YesSql should not be called in parallel, but does nothing but throw an exception if you do. OC has no mitigation, protection, or graceful degradation for the problem, and just allows the website to crash out. So, the application must handle the issue. In this case, by queuing the async requests so they are not called in parallel. How else could it be resolved?

[Piedone]

What calls YesSql in parallel is your code. The fix needs to be done there.

After #16079, in 2.0 we'll have an explicit exception for unsupported calls like this.

If, as Mike said, you have suggestions that would improve Blazor support, then that's always welcome. This PR was welcome as well, since we could continue the discussion toward a proper solution, it's just not the solution yet itself. E.g. if a change in OC would help you prevent that concurrent call, then that's a solution we can accept.