Evaluate if MultipleActiveResultSets is indeed necessary for SQL Server connections

[Piedone]

Is your feature request related to a problem? Please describe.

For SQL Server, you have to specify MultipleActiveResultSets=True; in you connection string, otherwise Workflows will fail with "Putting the workflow in the faulted state. System.InvalidOperationException: The connection does not support MultipleActiveResultSets". There are apparently multiple cases of this too.

However, I don't think we actually have to require MARS, and it should only be necessary if you do parallelized DB queries via the same DB connection, what you shouldn't.

Describe the solution you'd like

Figure out if we indeed need MARS, or we have unwanted parallelization. If the latter, then let's fix those. If we need MARS, then let's add validation like this: #15210

Describe alternatives you've considered

I can't think of anything else.

[deanmarcussen]

when this error occurs (and I have never seen it with workflows, which we use constantly), it tends to be a missing await somewhere, i.e. the session is being treated as thread safe BUT it is not

There have been some hints that calling multilple .SaveChangesAsync() may cause the above error (came from one of my team), but we refactored out the code that was doing that, so cannot comment further)

[sebastienros]

All the issues you pointed to are closed. Can you describe a simple workflow that will make it fail so we can track the issue?

[Piedone]

I did some tests with the latest source and can't find any issues. I'll remove MultipleActiveResultSets=True; from our prod apps' connection strings after upgrading to OC 1.8 and close this if there's still no issue.

[Piedone]

Hmm, was it rather with Audit Trail? I also did some quick checks with that but no issues.

The mentioned 1.8 upgrade of ours is pending.

[deanmarcussen]

Honestly Zoltan I would check any custom code you have.

Everytime this has been reported (you can search the issues) it has been a missing await or an async void usage, or an odd combination of .Result usage

[Piedone]

Most possibly yeah, I'll get back here once we have some experience with 1.8 in prod.

[deanmarcussen]

cool, it happened to me recently when visual studio automatically added an async to an existing method that was already void, and because it was error handling code we didn't see it for three months until the error actually happened, and it went down into that await
Disapointed none of the analzyers that you guys setup and we use didn't pick up on it :( (not your guys fault, just such an obvious thing for an analyzer to spot and complain about.)

[Piedone]

That's strange, because at least one of the analyzers should catch exactly this, chiefly AsyncFixer...

[deanmarcussen]

Best I have a look a the AsyncFixer and see if it's badly configured then... thx

[Piedone]

@sarahelsaig could you please add here what you found with the Roles module?

[sarahelsaig]

Sorry, I didn't notice your message. I have opened an issue about it yesterday here.

[Piedone]

OK, thanks. I'll close this less specific issue if we still don't find any MARS problems with 1.8.2 (upgrading DotNest is still in progress).

[MikeAlhayek]

@Piedone you should keep this open. MultipleActiveResultSets for some reason is required for a reason that I have not explained yet.

@deanmarcussen Here are steps of how reproduce this issue (assuming you already have a default tenant with SaaS recipe)

1. Create a new tenant (use SQL Server) database NOT SQLite. and Using Blog recipe
2. Try setting up the tenant
3. During the setup "half way there" I think during installing the Roles module, you'll encounter an InvalidOperationException exception thrown by RoleUpdater

Here is the exception

OrchardCore.Environment.Shell.ShellFeaturesManager|ERROR|IFeatureEventHandler thrown from OrchardCore.Roles.Services.RoleUpdater by InvalidOperationException System.InvalidOperationException: The connection does not support MultipleActiveResultSets.
   at System.Threading.Tasks.ContinuationResultTaskFromResultTask`2.InnerInvoke()
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
--- End of stack trace from previous location ---
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Task& currentTaskSlot, Thread threadPoolThread)
--- End of stack trace from previous location ---
   at Dapper.SqlMapper.QueryAsync[T](IDbConnection cnn, Type effectiveType, CommandDefinition command) in /_/Dapper/SqlMapper.Async.cs:line 434
   at YesSql.Store.ProduceAsync[T,TState](WorkerQueryKey key, Func`2 work, TState state)
   at YesSql.Services.DefaultQuery.Query`1.FirstOrDefaultImpl()
   at YesSql.Services.DefaultQuery.Query`1.FirstOrDefaultImpl()
   at OrchardCore.Data.Documents.DocumentStore.GetOrCreateMutableAsync[T](Func`1 factoryAsync) in C:\Code\OrchardCMS\OrchardCore\src\OrchardCore\OrchardCore.Data.YesSql\Documents\DocumentStore.cs:line 38
   at OrchardCore.Documents.DocumentManager`1.GetOrCreateMutableAsync(Func`1 factoryAsync) in C:\Code\OrchardCMS\OrchardCore\src\OrchardCore\OrchardCore.Infrastructure\Documents\DocumentManager.cs:line 71
   at OrchardCore.Roles.Services.RoleUpdater.UpdateRolesForInstalledFeatureAsync(IFeatureInfo feature) in C:\Code\OrchardCMS\OrchardCore\src\OrchardCore.Modules\OrchardCore.Roles\Services\RoleUpdater.cs:line 65
   at OrchardCore.Modules.InvokeExtensions.InvokeAsync[TEvents,T1](IEnumerable`1 events, Func`3 dispatch, T1 arg1, ILogger logger) in C:\Code\OrchardCMS\OrchardCore\src\OrchardCore\OrchardCore.Abstractions\Modules\Extensions\InvokeExtensions.cs:line 133    at System.Threading.Tasks.ContinuationResultTaskFromResultTask`2.InnerInvoke()
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
--- End of stack trace from previous location ---
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Task& currentTaskSlot, Thread threadPoolThread)
--- End of stack trace from previous location ---
   at Dapper.SqlMapper.QueryAsync[T](IDbConnection cnn, Type effectiveType, CommandDefinition command) in /_/Dapper/SqlMapper.Async.cs:line 434
   at YesSql.Store.ProduceAsync[T,TState](WorkerQueryKey key, Func`2 work, TState state)
   at YesSql.Services.DefaultQuery.Query`1.FirstOrDefaultImpl()
   at YesSql.Services.DefaultQuery.Query`1.FirstOrDefaultImpl()
   at OrchardCore.Data.Documents.DocumentStore.GetOrCreateMutableAsync[T](Func`1 factoryAsync) in C:\Code\OrchardCMS\OrchardCore\src\OrchardCore\OrchardCore.Data.YesSql\Documents\DocumentStore.cs:line 38
   at OrchardCore.Documents.DocumentManager`1.GetOrCreateMutableAsync(Func`1 factoryAsync) in C:\Code\OrchardCMS\OrchardCore\src\OrchardCore\OrchardCore.Infrastructure\Documents\DocumentManager.cs:line 71
   at OrchardCore.Roles.Services.RoleUpdater.UpdateRolesForInstalledFeatureAsync(IFeatureInfo feature) in C:\Code\OrchardCMS\OrchardCore\src\OrchardCore.Modules\OrchardCore.Roles\Services\RoleUpdater.cs:line 65
   at OrchardCore.Modules.InvokeExtensions.InvokeAsync[TEvents,T1](IEnumerable`1 events, Func`3 dispatch, T1 arg1, ILogger logger) in C:\Code\OrchardCMS\OrchardCore\src\OrchardCore\OrchardCore.Abstractions\Modules\Extensions\InvokeExtensions.cs:line 133

The RoleUpdater class does a look up in the database of a RoleDocument here which causes the exception.

[Piedone]

I was about to close this today, actually, because we haven't found any issues with 1.8. Are you saying that there's something else apart from #15794 and #15628?

[MikeAlhayek]

@Piedone I think solving #15794 may solve this issue too "but not sure". This ticket is about whether or not we should use MultipleActiveResultSets. Since MultipleActiveResultSets is disabled by default in Sql Server, I think we should not rely on enabling it for OC to run.

In my last comments, I showed by "as of now" this must be turned on otherwise we have a problem.

@deanmarcussen stated the following

Everytime this has been reported (you can search the issues) it has been a missing await or an async void usage, or an odd combination of .Result usage

I think there is a real problem that we should look at here and ensure that MultipleActiveResultSets is not needed

[Piedone]

I'll keep an eye out for it but I think this issue is invalid. In 1.8 we don't have any problems. In main we do, but those are tracked separately already with more specific issues too.

[deanmarcussen]

Probably the key takeaway here, is that YesSql is not threadsafe.

When you get an error message from sql that MultipleActiveResultSets must be enabled, it has tended to be, that something is trying to use YesSql in a non threadsafe manner.

I can't comment between the different versions of orchard involved, as we are currently on 1.6 and will remain there quite happily for some time.

Good luck, never an easy issue to track down.

[Piedone]

Yeah, that's what we figured under the related issues. Let's close this and focus on the two other specific issues.