Possible concurrency issues with caches?

[JamalQasem]

We are using SqlKata/Dapper(2.1.35) to implement a Web-API for a Postgres database with
around 20 tables. The API contains methods to query and manipulate data. It does not use
stored procedures. We test the repository methods of the API with around
200 unit tests, which run transactions against a test database rolling them back instead of committing.

Recently one of our tests started failing with the following error message:
Npgsql.PostgresException : 23503: insert or update on table "___" violates foreign key constraint "___"

We then annotated the offending test case with

[TestFixture, RequiresThread(ApartmentState.STA)]

and the problem vanished. All our repository methods rely on functions like

 protected async Task<IEnumerable<TEntity>> QueryAsync(Query query, IDbTransaction transaction, CancellationToken token)
 {
 var command = CompileQuery(query, transaction, token);
 return await transaction.Connection!.QueryAsync<TEntity>(command).ConfigureAwait(false);
 }

 private async Task<int> ExecuteAsync(Query query, IDbTransaction transaction, CancellationToken token)
 {
 var command = CompileQuery(query, transaction, token);
 return await transaction.Connection!.ExecuteAsync(command).ConfigureAwait(false); 
 }

We first checked that the queries use different connections of the connection pool and experimented
with the command flag NoCacheto no avail.
We finally decided to serialize the access using a semaphore.
This together with a call of SqlMapper.PurgeQueryCache(); made the failing test working again in MT.

The current implementation looks as follows

 protected async Task<IEnumerable<TEntity>> QueryAsync(Query query, IDbTransaction transaction, CancellationToken token)
 {
 await _semaphore.WaitAsync().ConfigureAwait(false);
 try
 {
 var command = CompileQuery(query, transaction, token);
 return await transaction.Connection!.QueryAsync<TEntity>(command).ConfigureAwait(false);
 }
 finally
 {
 SqlMapper.PurgeQueryCache();
 _semaphore.Release();
 }
 }

We suspect that we somehow manage to use different statements which map
to the same cache Identity so that concurrently running queries may use
a wrong cache entry. Leaving us with 2 unanswered questions:

• Did anyone run into the same issue?
• Is there a way to completely avoid caching?

[mgravell]

Hmm; this is odd. There are ways to bypass all the ref-emit caching; the one I'd say "try first", however, is the DapperAOT work. This is a little hard to opine on in this specific case because of your Query type, which is an unknown to me. The other option is the NoCache flag, which you say you have already been using.

However, I wonder whether we've ruled out simpler causes, such as the data genuinely violating FK rules for data reasons, unrelated to Dapper.

[JamalQasem]

Thank you very much for your swift reply.

Hmm; this is odd. There are ways to bypass all the ref-emit caching; the one I'd say "try first", however, is the DapperAOT
work.

As far as we know DapperAOT does not support all functionalities that Dapper provides.

This is a little hard to opine on in this specific case because of your Query type, which is an unknown to me.

The used Query type originates from the SqlKata query builder and generates a SQL command string which is injected in the
Dapper command definition like so:

var statement = _compiler.Compile(query).ToString();
return new CommandDefinition(statement, transaction:transaction, 
 cancellationToken:token, flags:flags);

The other option is the NoCache flag, which you say you have already been using.

NoCache seems to work for us with PR #2118.

However, I wonder whether we've ruled out simpler causes, such as the data genuinely violating FK rules for data reasons, unrelated to Dapper.

The FK violation is the result of the previous call to MultiMapAsync silently returning wrong data.

[goerch]

We suspect that we somehow manage to use different statements which map
to the same cache Identity so that concurrently running queries may use
a wrong cache entry.

@mgravell: we were able to reduce the failing scenario to two test cases running the same statement concurrently. Unfortunately these seem to have identical Dapper Identity hash codes although the associated DbDataReaders are different. It looks to me as if SqlMapper's GetColumnHash() computation doesn't incooperate the actual reader? We would be grateful about advice how to proceed.

[JamalQasem]

Unfortunately these seem to have identical Dapper Identity hash codes although the associated DbDataReaders are different. It looks to me as if SqlMapper's GetColumnHash() computation doesn't incooperate the actual reader?

A change like

 private static int GetColumnHash(DbDataReader reader, int startBound = 0, int length = -1)
 {
 unchecked
 {
 int max = length < 0 ? reader.FieldCount : startBound + length;
 int hash = -37 * (Thread.CurrentThread.GetHashCode() + startBound) + max;
 for (int i = startBound; i < max; i++)
 {
 object tmp = reader.GetName(i);
 hash = (-79 * ((hash * 31) + (tmp?.GetHashCode() ?? 0))) + (reader.GetFieldType(i)?.GetHashCode() ?? 0);
 }
 return hash;
 }
 } 

passes the Dapper test suite and seems to solve our original problem.

[mgravell]

@JamalQasem incorprating a thread-identifier into the hash simply masks the problem - it is artificially forcing more lookup misses, by only allowing requests on the same thread to ever equate - however, if there is an equality collision problem, it does nothing to prevent the actual problem if it occurs from the same thread. So: the worst of every outcome, even if it passes tests.

However: I am very open to us needing to radically rework the check here. Identity already incorporates a number of high-level checks including the SQL, parameter type, result type, and even connection-string metadata (in case of different databases having different schemas). The column-hash here is intended as a final smoke test along the lines of "did somebody run a DDL command on the database and change the schema in between two commands?". But: if that isn't working well in a scenario: let's explore that and fix it.

So: can I get more info about the scenario here? In particular, because Identity already includes those aforementioned checks, the only way I can imagine this happening is with a multi-purpose stored procedure that returns radically different shapes for different inputs, i.e. where MyProc @Mode=1, @Name='abc', @Id=123 returns radically different columns to MyProc @Mode=2,@Name='abc',@Id=123, and where by some complete fluke: the column hash comes back as identical.

The "real" solution here is to radically rework the way the column lookup works: we've already done that work over in AOT, which is why I say that if possible, "move to AOT" may be a quick fix. It is stupendously hard to fully rework that into Dapper "vanilla" (see #1909), however, we might be able to do some compromise that fixes the underlying problem. For example, instead of a column hash, we could store a string of the column type data for "new" records, compare against that full string (ideally without actually allocating a string each test time, but that's an implementation detail - either via forwards crawl, or "generate to a buffer, compare buffers").

So: @goerch - I want to explore this, but to do that: do you have a concrete example I can look at, that fails?. In particular, is my "SP that returns different things in different contexts" guess even remotely correct? This would also seem to be consistent with your mention of "the same statement concurrently"; again, normally expects that the same statement, with the same input and output types, is going to have the same fundamental shape. If that expectation is wrong: we can fix it.

[goerch]

@mgravell: many thanks for your support!

do you have a concrete example I can look at, that fails?

We are currently lucky(?) enough that our problem occurs rather deterministically, which is not a given in MT enviroments. We'll next try to reduce and anonymize the tests, which will need a bit of time because some infrastructure components are involved.

[goerch]

@mgravell: we have published a reduced test case which hopefully helps to reproduce the issue.

[goerch]

Unfortunately our analysis so far seems to be incomplete or wrong: even with full serialization and purging the cache we started to see the problem in our production code. I'm out of my depth trying to investigate the details of Dapper deserialization. Our workaround for now is to use a simple implementation of the needed Dapper interfaces using ADO.NET and basic reflection.

[jnm2]

I found this happening in production. There is evidence both of a torn write and a torn read happening with the SqlMapper.CacheInfo.Deserializer field which holds a value of the following struct:

Dapper/Dapper/SqlMapper.DeserializerState.cs

Lines 9 to 19 in 72a54c4

	private readonly struct DeserializerState
	{
	public readonly int Hash;
	public readonly Func<DbDataReader, object> Func;
	public DeserializerState(int hash, Func<DbDataReader, object> func)
	{
	Hash = hash;
	Func = func;
	}
	}

The tearing is manifesting as System.Data.DataException: Error parsing column 4 (Xyz=123 - Int32) ---> System.InvalidCastException: Unable to cast object of type 'System.Int32' to type 'System.String'. at Deserialize54e97abb-b5a4-4c03-ab44-488137ba6863(IDataReader )

For the torn write, one process had a runaway streak of these exceptions and had to be restarted. For the torn read, this exception happened as a one-off. The opportunity for a torn write or torn read can be seen in several places, e.g.:

Dapper/Dapper/SqlMapper.GridReader.Async.cs

Lines 202 to 213 in 72a54c4

	if (await reader.ReadAsync(cancel).ConfigureAwait(false) && reader.FieldCount != 0)
	{
	var typedIdentity = Identity.ForGrid(type, index);
	CacheInfo cache = GetCacheInfo(typedIdentity, null, addToCache);
	var deserializer = cache.Deserializer;
	int hash = GetColumnHash(reader);
	if (deserializer.Func is null || deserializer.Hash != hash)
	{
	deserializer = new DeserializerState(hash, GetDeserializer(type, reader, 0, -1, false));
	cache.Deserializer = deserializer;
	}

I've reproduced both the torn read and the torn write with a noninvasive unit test. PR incoming.

How a torn write happens

This is the worst issue because a torn write has actually corrupted the state of the SqlMapper.CacheInfo.Deserializer field indefinitely, breaking all calls that match the hash until either the grid reader runs into a result set with a different schema hash, or the process restarts.

Taking the code above as an example, the culprit is the line cache.Deserializer = deserializer; and the fact that the value being assigned is a struct with two fields. Struct assignments are not guaranteed to be atomic (side note: not even structs wrapping a single primitive field)!

https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/language-specification/variables#96-atomicity-of-variable-references (emphasis mine):

Reads and writes of the following data types shall be atomic: bool, char, byte, sbyte, short, ushort, uint, int, float, and reference types. In addition, reads and writes of enum types with an underlying type in the previous list shall also be atomic. Reads and writes of other types, including long, ulong, double, and decimal, as well as user-defined types, need not be atomic. Aside from the library functions designed for that purpose, there is no guarantee of atomic read-modify-write, such as in the case of increment or decrement.

Thus, because DeserializerState is a struct and not a class and no locking or equivalent synchronization is done around every read and write, the innocent-looking line cache.Deserializer = deserializer; causes a non-atomic two-part assignment to the backing field of the Deserializer property. Here's what happens when two threads hit this line at the same time with different hashes:

1. Thread A writes hash A into the Hash field inside the cache.Deserializer backing field.
   • New state: [hash A] [func X]
2. Thread B writes hash B into the Hash field inside the cache.Deserializer backing field.
   • New state: [hash B] [func X]
3. Thread B writes func B into the Func field inside the cache.Deserializer backing field.
   • New state: [hash B] [func B]
4. Thread A writes func A into the Func field inside the cache.Deserializer backing field.
   • New state: [hash B] [func A]

All the writes are now finished, and the cache.Deserializer backing field is now corrupted in a way that will pair an inappropriate deserialization func with all future queries whose schema matches that hash.

This is only one way the torn write can happen. Since writes are not totally ordered, it's possible to have the field writes published in either order (hash first or func first).

How a torn read happens

A torn read can happen in the code above when thread A is performing the non-atomic struct copy var deserializer = cache.Deserializer; while thread B is performing the non-atomic struct copy cache.Deserializer = deserializer;.

1. Thread A reads hash A from the Hash field inside the cache.Deserializer backing field.
   • New state of var deserializer: [hash A] [uninitialized]
2. Thread B writes hash B into the Hash field inside the cache.Deserializer backing field.
3. Thread B writes func B into the Func field inside the cache.Deserializer backing field.
4. Thread A reads func B from the Func field inside the cache.Deserializer backing field.
   • New state of var deserializer: [hash A] [func B]

This is only one way the torn read can happen. Since writes are not totally ordered, it's possible to have the field writes published in either order (hash first or func first).