Use strings as identifiers in the in-memory database rather than model references

[ajcvickers]

This test failed on one run on my machine. Probably a test concurrency issue; could be a concurrency issue inside EF/the in-memory provider.

Failed tests
0.4861182s✘ Microsoft.EntityFrameworkCore.IntegerValueGeneratorTest.Each_property_gets_its_own_generator_with_seeding
Assert.Equal() Failure\r\nExpected: 3\r\nActual:   1
   at Microsoft.EntityFrameworkCore.IntegerValueGeneratorTest.Each_property_gets_its_own_generator_with_seeding() in C:\github\efcore\test\EFCore.InMemory.FunctionalTests\IntegerValueGeneratorTest.cs:line 106
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)

[roji]

Not sure this is the same, but I just what looks like a flaky in-memory test failure in CI:

System.NullReferenceException : Object reference not set to an instance of an object.
   at Microsoft.EntityFrameworkCore.ConfigPatternsInMemoryTest.Can_save_and_query_with_implicit_services_and_OnConfiguring() in /_/test/EFCore.InMemory.FunctionalTests/ConfigPatternsInMemoryTest.cs:line 22
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)

[roji]

Another one:

Insert_update_and_delete_with_wrapped_Uri_key

System.InvalidOperationException : Sequence contains no elements
   at System.Linq.ThrowHelper.ThrowNoElementsException()
   at lambda_method402224(Closure)
   at Microsoft.EntityFrameworkCore.InMemory.Query.Internal.InMemoryQueryExpression.ResultEnumerable.GetEnumerator() in /_/src/EFCore.InMemory/Query/Internal/InMemoryQueryExpression.Helper.cs:line 21
   at Microsoft.EntityFrameworkCore.InMemory.Query.Internal.InMemoryShapedQueryCompilingExpressionVisitor.QueryingEnumerable`1.Enumerator.MoveNextHelper() in /_/src/EFCore.InMemory/Query/Internal/InMemoryShapedQueryCompilingExpressionVisitor.QueryingEnumerable.cs:line 170
   at Microsoft.EntityFrameworkCore.InMemory.Query.Internal.InMemoryShapedQueryCompilingExpressionVisitor.QueryingEnumerable`1.Enumerator.MoveNext() in /_/src/EFCore.InMemory/Query/Internal/InMemoryShapedQueryCompilingExpressionVisitor.QueryingEnumerable.cs:line 118
   at System.Linq.Enumerable.TryGetSingle[TSource](IEnumerable`1 source, Boolean& found)
   at lambda_method402223(Closure, QueryContext)
   at Microsoft.EntityFrameworkCore.StoreGeneratedInMemoryTest.Insert_update_and_delete_with_wrapped_Uri_key() in /_/test/EFCore.InMemory.FunctionalTests/StoreGeneratedInMemoryTest.cs:line 2236
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)

[ajcvickers]

So I figured out what is going on here. Essentially, the in-memory tests are filling the IMemoryCache with models to the extent that the cache size is exceeded, and this causes some models to be purged. This means that some tests get a new model between two uses of the same context type. This in turn means that the IEntityType instance which is used to lookup the table in the in-memory database changes, and hence the data is not found.

This only happens when IEntityType instances are being used to lookup tables in the in-memory database--if entity type names are used, then there isn't a problem, since both the old and new model have entity types with the same name. We might consider making this the default--we should discuss.

For now, I will push two PRs: one with the debugging in the model cache so others can look, and one that increases the cache size to unblock the C.I. This is likely not a good long-term solution--we should likely change the in-memory database implementation instead.

It's interesting that it looks like the way MemoryCache works, or the way we use it, seems to have changed here--it's reaching the size limit much more quickly than it ever has before. I get failures even with a cache size five times more than it was.

/cc @roji @eerhardt

[roji]

Well done tracking this down... makes sense!

If you're able to repro this relatively easily, then we can bisect to see when it started and possibly track down the cause for the change.

In an ideal world, maybe it this could be managed via EnsureCreated/EnsureDeleted like for normal databases, where the test explicitly drops the database when it's done; that would prevent relying on automatic LRU/purging logic which is unpredictable...

[roji]

(I wonder if this started triggering simply because we happened to add a ton more new tests at some point?)

[ajcvickers]

In an ideal world, maybe it this could be managed via EnsureCreated/EnsureDeleted like for normal databases

The in-memory database doesn't go away; that's what was hard to debug about this. However, the database uses reference equality on model types to key tables. So if the model changes, the items are not found in the database even though the database exists and still contains the data. I think this is the bit we should change to be more like a normal database; the keys for the tables should be derived from the model types, but not ever be the model types themselves. We already have this with the "_useNameMatching", but the key probably needs to be stronger than just the type name if we are to make it the default.

If you're able to repro this relatively easily, then we can bisect to see when it started and possibly track down the cause for the change.

I have a hunch what the cause was which I will investigate. :-)

I think we also need to talk about using a single MemoryCache for queries and models, and whether we should allow explicit removal of models from the cache when the application says they are no longer used.

[ajcvickers]

Additional info:

• Single cache is used for compiled queries, relational commands, and models
• Traditional default size of the cache is 10240.
• Compiled queries and relational commands are added with a size of 10 and default priority.
• Models are added with the size of 100 and high priority.

So it seems like an application that creates a lot of models (which should be uncommon, but we know some people do this for multi-tenancy) will quickly fill its cache with models and start evicting queries.

[ajcvickers]

This is the root cause: #30365

[AndriySvyryd]

We also need to (lazily) invalidate the cached queries if the corresponding model is no longer cached.

[ajcvickers]

Notes from design meeting:

• Update in-memory database to key by strings created from model types, rather than the model types themselves (This issue)
• Create separate caches for models and queries Separate caches for models and queries #30476
• Add public API to evict model from the cache Public API to evict models from the cache #30475
• Either stop evicting models from the cache (and fail if it fills) or make sure that eviction is safe detected by the query caches Separate caches for models and queries #30476