Fixes Concurrency and MARS issues

[jtkech]

Fixes #7449

When updating a shared document, if its CheckConsistency option is true (as it is by default), after the session is committed successfully, we query again the document for consistency checking.

On YesSql side, this query calls YesSql.Store.ProduceAsync() that may throw a MARS exception.

InvalidOperationException: The connection does not support MultipleActiveResultSets.
Microsoft.Data.SqlClient.SqlCommand+<>c.<ExecuteDbDataReaderAsync>b__164_0(Task<SqlDataReader> result)
...
Dapper.SqlMapper.QueryAsync<T>(IDbConnection cnn, Type effectiveType, ...) in SqlMapper.Async.cs
YesSql.Store.ProduceAsync<T>(WorkerQueryKey key, Func<object[], Task<T>> work, object[] args)
YesSql.Services.DefaultQuery+Query<T>.FirstOrDefaultImpl()

So here, if the session is already committed, we create a new light scope on the existing current context (which is fast), and just to resolve a new session, then we use it to get back the document for consistency checking.

In #7449 the original issue of @giannik will be fixed through sebastienros/yessql#287

But i have an idea how to temporarily fix it in OC in the meantime, if it works i will update this PR so that it will also fixes #7545. Update: Okay, defining a Version property in our Document base class prevents a version missmatch

[deanmarcussen]

@jtkech I had a look at YesSql and there is a test ShouldKeepIdentityMapOnCommitAsync which suggests that we shouldn't lose the original document from the Identity Map.
But it uses GetAsync() rather than a query, which would make sense of why it gets lost or does a requery.

Would it be possible, to internally keep a track of the Document on Commit, so that GetOrCreateImmutableAsync just returns the current document from the scope of the store, rather than having to force out a new query?

I see you already have a loaded dictionary, but are clearing it, because it's for a different purpose.

One of my concerns is that in doing a second query, you might actually pick up a document from a change on another server, but it would then save, and bypass the real concurrency exception.

Happy to test it, if it might be a better solution.

This currently isn't blocking or anything, it just reveals a bit of weirdness that is easy to work around.

[jtkech]

@deanmarcussen

For info this GetOrCreateImmutableAsync() is called after the session is committed for consistency checking, related to the CheckConsistency option (not CheckConcurrency), it only happens if the session is committed successfully (e.g. no concurrency exception), if so we set the cache and, if the related option is true, we call it to retrieve what was really persisted so that we can compare identifiers for consistency checking.

Would it be possible, to internally keep a track of the Document on Commit, so that GetOrCreateImmutableAsync just returns the current document from the scope of the store, rather than having to force out a new query?

But here we want to retrieve what was really persisted after the session was committed sucessfully.

I see you already have a loaded dictionary, but are clearing it, because it's for a different purpose.

Not for a different purpose, for the same reason, to retrieve what was really persisted

One of my concerns is that in doing a second query, you might actually pick up a document from a change on another server , but it would then save, and bypass the real concurrency exception.

This is exactly what we want to do, in that case we invalidate the cache we just set before

We use the pattern where, just after storing the document, we set the cache, another pattern would be to invalidate the cache so that it will be refreshed with a query on a future demand, this last pattern is more consistent but less efficient for front end requests. So, in fact we use a mix, we set the cache immediately (after storing the document), so the first pattern, then just after we do a consistency checking that may invalidate the cache, so fallback to the 2nd pattern. The use case is when your are the last to set the cache, but not the last to set the store

[jtkech]

@deanmarcussen does this answer to your question?

[deanmarcussen]

ok, I get it - I think you have already explained this to me before as well ;)

So yes, as a consistency check, rather than a concurrency check, I see that you do need to make a seperate query to confirm the consistency.

YesSql.ProduceAsync is something we need to look at further I suspect.

This fixes the issue, which is good, but it concerns me that we need to make a new shell scope to do a query after a CommitAsync.

The concern is that as a module developer, when using event handlers, we have no way to know that CommitAsync has been called on a session, possibly partway through the request session, and that a query may fail to execute on a particular database provider.

i.e. we have various CommitAsync on User creation etc, and now here.

But all good for now, we should just understand more if there is something on YesSql we can improve.

[jtkech]

@deanmarcussen

YesSql.ProduceAsync is something we need to look at further I suspect.

Yes, normally here, it only does a sql select to read the doc, but maybe too soon just after the same session was committed, hmm interesting to add a delay here for testing, but was too lazy to re-use a local feed ;)

This fixes the issue, which is good, but it concerns me that we need to make a new shell scope to do a query after a CommitAsync.

Yes but here i create a scope on the current shell context (so not rebuilt if it was released) and just to resolve the ISession, so execute what we do when resolving it store.CreateSession() and so on, it is faster than the query itself, but yes would be better to not have to do it

The concern is that as a module developer, when using event handlers, we have no way to know that CommitAsync has been called on a session, possibly partway through the request session, and that a query may fail to execute on a particular database provider.

No, it is still the only one that we call asynchronously just before the shell scope is disposed, see how the ISession is resolved, we register on the shell scope a BeforeDispose delegate that resolves IDocumentStore to call the CommitAsync() that is defined here. So, not at the very end of the request scope but only once and at the end of the current shell scope where OC things are executed.

                ShellScope.RegisterBeforeDispose(scope =>
                {
                    return scope.ServiceProvider
                        .GetRequiredService<IDocumentStore>()
                        .CommitAsync();
                });

We can also register on IDocumentStore an AfterCommitSuccess and / or an AfterCommitFailure per document type. We use AfterCommitSuccess to read back the document for consistency.

i.e. we have various CommitAsync on User creation etc, and now here.

Yes User is an exception, it was the first to use checkConcurrency and then a try / catch on committing, maybe we could deffer it, as we do with the new AfterCommitFailure but that right now can only be used for an IDocument.

The only other exception is the WF commit task that we created but for another reason. But here we don't add a new one, as said above the CommitAsync() defined here is still the only one we call before the scope is disposed.

But all good for now, we should just understand more if there is something on YesSql we can improve.

Yes ;)

[jtkech]

Just for infos, just tried some tests

It also works with the following, so without creating a new scope, but still using a new session. Here one difference is that we don't register scoped indexes, but i think we don't need them for any shared IDocument.

            using var session = _session.Store.CreateSession();
            document = await session.Query<T>().FirstOrDefaultAsync();
            await session.CommitAsync();

I added a delay before retrieving the doc, with different delays i got different errors: Connection is closed, There is already an open DataReader, but with a delay > about 300ms that's okay. Hmm, this looks like a non awaited task ;) Hmm, to repro (and to mimic the use case of @jptissot ) i needed to add a role step in the menu migration recipe.

Need to leave but i will check it this night.

Anyway, it seems that we will still need a new session to not retrieve an entity still cached on YesSql side, even after the session was committed, otherwise the consistency checking would be useless.

Update: Just did a last test, it is the same issue by adding a MediaProfiles step (not the roles step), so not related to the roles, i prefer this. Hmm, now i remember i already saw this kind of issue, that why we execute each recipe step in its own shell scope, which is not the case for the steps of a **migration recipe **, that would need to only do migrations, only updating the type definitions table.

[jtkech]

@deanmarcussen @sebastienros just for infos

Okay i found the non awaited calls, if you have an async delegate on which multiples ones was registered through the += operator, in place of doing only one async call on it, you need to do a foreach on its GetInvocationList(), otherwise there are not awaited from each others.

So we needed to add a Version property to our Document to fix the CheckConcurrency issue, but @sebastienros reviewed my suggestion on YesSql and upated it, so we will be able to remove this property with a next YesSql version.

And the MARS issue will be fixed from the source, but i will still use a new session for consistency checking to not retrieve a cached entity from YesSql, but without creating a shell scope, just by using the session store CreateSession() instead.

I have another little concern about migration recipes but it's enough for the moment ;)

I will do a PR soon.