AsyncLocal leak in SystemTransactionContext

[pecanw]

The SystemTransactionContext uses following field: private readonly AsyncLocal<bool> _bypassLock = new AsyncLocal<bool>(); By design, each creation of the 'SystemTransactionContext' (specifically the '_bypassLock' field) adds a new item into ExecutionContext._localValues dictionary and each assignment to _bypassLock.Value creates a copy of ExecutionContext._localValues dictionary.

So if you run many transactions in one execution context (it can be a background job thread or main thread in a console application) the ExecutionContext._localValues dictionary gets bigger and bigger which causes

1. Memory leak issues
2. Performance issues (as assigning to the '_bypassLock.Value' creates a copy of dictionary with a lot of items).

I have checked the code and at first glance it seems to me that the _bypassLock field could be made static as each thread gets its own copy of the execution context. I have created a unit test and a proposed fix (see pull request 1596).

However I understand that the unit test is not 100% correct as it's using reflection to get the .NET system private dictionary (ExecutionContext._localValues) which might change in future versions of .NET, but I don't have an idea how to write the test more correctly.

Please check the proposed solution and / or comment the issue.

[fredericDelaporte]

By design, each creation of the 'SystemTransactionContext' (specifically the '_bypassLock' field) adds a new item into ExecutionContext._localValues dictionary

Reading from source, this statement is inexact. Dictionary<IAsyncLocal, object> ExecutionContext._localValues gets appended with a new key/value only when the async local value is affected, which occurs only at transaction completion.

This still means each transaction context ends up at some point appending an entry to this dictionary currently.

each assignment to _bypassLock.Value creates a copy of ExecutionContext._localValues dictionary

Indeed. It first copies the dictionary then appends/alters the copy with the async local as key and its value as value.

So if you run many transactions in one execution context (it can be a background job thread or main thread in a console application) the ExecutionContext._localValues dictionary gets bigger and bigger which causes

1. Memory leak issues
2. Performance issues (as assigning to the '_bypassLock.Value' creates a copy of dictionary with a lot of items).

This looks as a serious flaw of AsyncLocal, especially since nothing warns about it needing to be very sparingly created on the same thread.

I have checked the code and at first glance it seems to me that the _bypassLock field could be made static as each thread gets its own copy of the execution context.

This is a bit counter-intuitive for the case where two sessions are used in the same thread, both with distributed transactions causing them to complete concurrently on additional threads. But that should work, since the execution context is cleared when a thread is reused for another task.

When the transactions are not distributed, it then requires the value to be reset to false at the end of the transaction completion, not at the context creation. Because we can open a transaction on both sessions simultaneously, so the first to be completed needs to set the lock value to false for the next already opened. (The Unlock method should be a good place for this.)

However I understand that the unit test is not 100% correct as it's using reflection to get the .NET system private dictionary (ExecutionContext._localValues) which might change in future versions of .NET, but I don't have an idea how to write the test more correctly.

In .Net Core this field seems to be named m_localValues, so reflection on _localValues will not work when testing .Net Core.

[pecanw]

As for adding the new item into the ExecutionContext._localValues you are absolutely right. I had investigated the .NET sources and then I wrote the comment inexactly. However, as you said - the dictionary gets "flooded" anyway.

Based on you comments I have added the _bypassLock.Value = false; to the Unlock method. and removed it from the constructor as it is not necessary to initialize it with a default value (I have checked this in the .NET sources).

As for the unit test, I have added a conditional compilation to address the issue. Anyway I don't like the fact of relying on some private field name... Do you have another idea how to address it?

[fredericDelaporte]

Anyway I don't like the fact of relying on some private field name...

For tests, we avoid doing it if possible, but accept it when needed.

Do you have another idea how to address it?

No, I do not.