zzzprojects/EntityFramework.DynamicFilters

Concurrency error when PreventDisabledFilterConditions is called

prvyk opened this issue · 4 comments

prvyk commented

System.InvalidOperationException: Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct.
at System.Collections.Generic.HashSet`1.AddIfNotPresent(T value)
at
EntityFramework.DynamicFilters.DynamicFilterExtensions.PreventDisabledFilterConditions(DbModelBuilder modelBuilder, String filterName)

Affected software is a ASP.NET Core 3.1 web API using EFv6.44, a restart stopped the error from recurring. I've been using the package for several years (most of the time with .NET Framework) without this issue, this must be a very rare race condition.
I guess the fix is using ConcurrenctDictionary<> Instead of HashSet<>, like the other variables in DynamicFilterExtensions class.

Hello @prvyk ,

Do you have many different contexts that could be instanced at the same time?

For example, a CustomerContext and InvoiceContext.

That's currently one way that I could think this issue can happen.

Best Regards,

Jon


Sponsorship
Help us improve this library

Performance Libraries
context.BulkInsert(list, options => options.BatchSize = 1000);
Entity Framework ExtensionsBulk OperationsDapper Plus

Runtime Evaluation
Eval.Execute("x + y", new {x = 1, y = 2}); // return 3
C# Eval FunctionSQL Eval Function

prvyk commented

Do you have many different contexts that could be instanced at the same time?

For example, a CustomerContext and InvoiceContext.

Hmm.

  • This API uses only one context, which should be disposed after each call.

  • However, logs indicate once the error condition appeared, it triggered on every call on different threads until the API was restarted (not just a particular thread or call, though the caller retried frequently).

  • The API uses the Ambient dbcontext approach. The particular ambience code relies on AsyncLocal which I believe is independent between different threads (unless it's a child thread which I believe doesn't apply here), so a different API call should always use a different context?

  • If for some reason that wasn't the case, I can see the failure scenario here: a race when called with just the 'right' timing, the bugged context being recalled each time, the caller retries frequently enough so the context isn't disposed 'naturally'.

Hello @prvyk ,

The v3.2.2 has been released.

In this version, we changed the HashSet for a ConcurrenctDictionary like you proposed.

Let me know if that fix your issue or you still have it.


Is this library useful to you? Please help us by becoming a sponsor to keep it alive and supported.

prvyk commented

Thanks for working on this package!

The code works for me. I couldn't find a scenario which would replicate the previous error, so it's not a complete test. But I don't see a problem now, and the fix should handle this if it recurs.