Troubles using LocalContext provided by ApplicationContextManager

[mirecg]

Hello, I'm migrating from Csla4 to Csla8. A big jump, I know.

But what's the question or maybe an issue I have?

ApplicationContext.LocalContext is provided by default IContextManager implementation which is ApplicationContextManager.
ApplicationContextManager uses AsyncLocal storage for both LocalContext and ClientContext.

In Csla4 the LocalContext was stored in TLS and is still supported in Csla8 by ApplicationContextManagerTls implementation.
So far so good.

For new applications with an async/await programming style, TLS storage is not working anymore, when we want to share context between async calls.

AsyncLocal enables to "flow" data from one task to another. So when we set something in appContext.LocalContext (appContext is ApplicationContext resolved from IServiceProvider) and make some async/await call or Task.Run(), we will get the same instance of LocalContext in invoked async calls/tasks.

The problem I see, is that the LocalContext is the value stored in AsyncLocal as a whole, so that when we initialize it in one task and invoke from there another async/await call, that call will "see" the same instance of LocalContext with the same values in it.
When we perform same parallel work the invoked task well run over the same instance of LocalContext with potential conncurrency problems.

See this code example with comments in it, which describes the problem better.

var appContext = serviceProvider.GetRequiredService<ApplicationContext>();

// Task#1
// this will store the ContextDictionary in AsyncLocal storage for this task,
// which will be propagated to other tasks invoked from this task
appContext.LocalContext["SharedState"] = "some state";
Console.WriteLine($"SharedState: {appContext.LocalContext["SharedState"]}");

// store the instance of LocalContext for comparison
var localContext = appContext.LocalContext;

// Perform parallel work on some work-items, depending on stored state in LocalContext
int counter = 0;
var items = Enumerable.Range(1, 1000);
Parallel.ForEach(items, item =>
{
	// Task#n
	var appContext2 = serviceProvider.GetRequiredService<ApplicationContext>();
	var localContextInInvokedTask = appContext2.LocalContext;				
	// will get the same instance of LocalContext as the invoking task, which may raise concurrency problems !!!
	if (Object.ReferenceEquals(localContextInInvokedTask, localContext))
	{
		// hence will contain the same value in ContextDictionary as invoking Task
		var sharedState = (string)appContext2.LocalContext["SharedState"];
		if (sharedState == "some state")
		{
			// changes the state, but this statement changes it globaly for other tasks that will be invoked after this moment !!!
			// and will be modified concurrenly, but ContextDictionary which is the LocalContext type, is not thread-safe !!!
			appContext2.LocalContext["SharedState"] = $"some other state from work-item #{item}";
			Interlocked.Increment(ref counter);
		}

		// and perform some work depending on LocalContext
		PerformWork(item);
	}
});

// here we are in Task#1, SharedState will contain some random state set from Parallel.ForEach() !!!
Console.WriteLine($"SharedState: {appContext.LocalContext["SharedState"]} (only {counter}/{items.Count()} threads changed the state!)");

I would assume that the LocalContext is implemented as ConcurrentDictionary<string, AsyncLocal<object>> which would behave more like synchronous style programming, e.g. invoking only sync calls, which shared the context in the call hiearchy:

• thread-safe
• only the individual key/value would be shared between async/await calls
• is compatible with classic/old style synchronous code which counts on/with the TLS like behaviour

[StefanOssendorf]

Hi, I've not used the Local/Client/*Context stuff but here are my thought on this:

• TLS is only thread safe for immutable types. As soon as you put a reference type (except string) in the TLS the instance is not thread safe (except it's implemented in a thread safe manner)
• How do you decide that? Would it make a copy? How should it make clone from a complex type? Deep clone, shallow clone?
• Can't say anything to that :D

In my opinion, since you are using the parallel you are responsible for making the stuff thread safe. Don't know what CSLA could provide here? It does not know if it's used in a parallel context or not. Futhermore you are resolving a new application context from the original DI scope which means - from a ApplicationContext perspective - you are still within one execution flow. So of course the "inner state" of the application context is preserved.

[Bowman74]

I like @StefanOssendorf idea simply because putting things behind interfaces instead of direct references to concrete objects makes the entire framework more extensible to handle whatever cases we have not thought of.

[Bowman74]

@rockfordlhotka and yes that was me volunteering if you want it done.

[rockfordlhotka]

Thank you @Bowman74 - here's the issue: #4015

[mirecg]

Hi, thank you for your quick response and understanding the problem with thread-safety.

I would like to elaborate on the second issue what I mentioned, that instead of storing whole ContextDictionary as AsyncLocal we should store individual values as AsyncLocal.

The current implementation looks like this, see ApplicationContextManager:
private readonly AsyncLocal<ContextDictionary> _localContext = new();

I would suggest to be the localContext implemented similar to this
private readonly ConcurrentDictionary<string, AsyncLocal<object>> _localContext = new();

In my opinion this is more suitable implementation, which isolates individual values between tasks, so that when two or more tasks are invoked from some executing task, those tasks will not see and more importantly they will not overwrite each other's values ​in LocalContext, even when they get the same instance of ContextDictionary.

[rockfordlhotka]

@mirecg That is an interesting idea.

The thing is, Local/Client context are managed and maintained in different ways across a number of application context manager implementations. We'd have to examine each of those implementations to determine if this is the right approach.

Also, and perhaps more important, is the fact that we need to preserve backward compatibility with the API - I shudder to think about breaking this widely-used API for everyone. So we'd have to figure out how to "unbox" each value when it is requested - and would that eliminate the thread safety we're looking for here?

Somehow when I do

var value = applicationContext.LocalContext["myvalue"];

I'd get back the actual value, not the wrapped AsyncLocal value. Does that mean I don't have thread protection on the value?

And honestly, even if we do wrap a complex type in AsyncLocal, if two threads start interacting with properties of that object instance, I don't think AsyncLocal will protect against that.

[rockfordlhotka]

@mirecg see the notes in #4015 regarding how it will now be possible (not required) for code to safely use the local/client context collections.

I'm sure all existing code uses the not-thread-safe methods and iterators - but that is "your" code, not CSLA code. Other than, as @Bowman74 notes, MobileFormatter. I am not concerned about MobileFormatter, because if someone is interacting with the object graph while it is being serialized they are already in such deep trouble as to where this won't matter.

Generally speaking it is important to understand that CSLA and CSLA-based domain objects are not thread safe. Like the vast majority of the BCL, there's no thread safety implied except where specifically documented.

[mirecg]

Hello, sorry for late reaction.

Hmm, it seems that this is not very well understood problem that I've raised. I do not know how better to explain in it, but I will try.

Firstly, I would like to comment your previous reply:

Somehow when I do
var value = applicationContext.LocalContext["myvalue"];
I'd get back the actual value, not the wrapped AsyncLocal value. Does that mean I don't have thread protection on the value?

here is where the instance of ContextDictionary get unwrapped from AsyncLocal<ContextDictionary>:

csla/Source/Csla/Core/ApplicationContextManager.cs

Lines 63 to 66 in 9a5c5e6

	public IContextDictionary GetLocalContext()
	{
	return _localContext.Value;
	}

when you acesss it for this first time on some Task, there will be null
here you can see, that when this happens, this code will create it and set it back to AsyncLocal<ContextDictionary>.Value in SetLocalContext method:

csla/Source/Csla/ApplicationContext.cs

Lines 82 to 94 in 7defe60

	public IContextDictionary LocalContext
	{
	get
	{
	IContextDictionary ctx = ContextManager.GetLocalContext();
	if (ctx == null)
	{
	ctx = new ContextDictionary();
	ContextManager.SetLocalContext(ctx);
	}
	return ctx;
	}
	}

When we return back to your comment, the value itself what I get from the LocalContext isn't thread-safe, yes, this is what "my" code should take care of it.
But what I found as a real problem, is that you can easily come to situation where two or more tasks get the same instance of ContextDictionary due to asynchronous flow of data encapsulated inside AsyncLocal<>.

See this simple example:

private static void Test_SameLocalContextInDifferentTasks(ApplicationContext applicationContext)
{
    // this will force to create instance of ContextDictionary for the current task
    var localContext = applicationContext.LocalContext;

    // simulate some work in parallel
    Parallel.ForEach(Enumerable.Range(0,10), _ => {
        var localContextInOtherTask = applicationContext.LocalContext;
                
        // the output will be True for all tasks,
        // because the LocalContext value is propagated from the invoking task due to AsyncLocal<T> behavior
        Console.WriteLine(Object.ReferenceEquals(localContextInOtherTask, localContext));
    });
}

There's another related problem. This is not only about thread-safety of ContextDictionary.
In this way as it is implemented, that the whole dictionary flows from one task to another and I'm unable to write logic in the above example, that would allow me to store different values under the same key inside LocalContext, because all those tasks share the same instance of dictionary, and will overwrite that value.
In practice, let's say I have some unit-of-work pattern for tracking execution of different code at different levels, let's call it simply session, which starts when you create instance of session, and ends when you dispose it.
To be able to get the session from some code that is executing, no matter how nested it is, I will store it inside LocalContext under the same key (for example "session") so that I will always get the same instance that is related to the current execution context.

[rockfordlhotka]

Maybe we need to back up and you can help me understand what you are trying to accomplish. It might be that LocalContext isn't the right answer for what you need.

[Bowman74]

It sounds like you want a local context per thread and local context as implemented in the context managers are not designed to do that. What it I starting to sound like is you need to create your own implementation of IContextManager so you can control when you get an instance of a new local context dictionary, not just make the existing dictionary thread safe.

As @rockfordlhotka stated we'd have to know more about your use case

[mirecg]

Hmm, it should be clear enough from what I've described so far.

I would like to see some reactions to may previous post. Don't you see from the simple code example what's going on?

What I'm trying to achieve is to have similar behaviour of LocalContext with TLS storage (as it was in Csla4), which is still available through ApplicationContextManagerTls, but also I wan't to use async programming style without a worry even when I would spawn several tasks in the way I've showcased above.

The behavior I expect is similar to what was CallContext from System.Messaging.Remoting, but it is not available in .NET Core versions and above.

Maybe you can read more about it here https://stackoverflow.com/questions/42242222/net-core-equivalent-of-callcontext-logicalget-setdata, which will point you also here https://www.cazzulino.com/callcontext-netstandard-netcore.html, and you will hopefully understand why I'm suggesting tu have LocalContext backing store implemented as ConcurrentDictionary<string,AsyncLocal<object>>.

[rockfordlhotka]

What you are suggesting is a massive breaking change, as everyone using the context values in their apps would all have to deal with AsyncLocal values instead of direct values. Unless you are aware of a way to provide an abstraction that wouldn't break every CSLA app out there?

You are right - .NET has changed, dropping remoting and call contexts, and really TLS is generally unavailable in so many modern .NET runtimes as well.

I don't know what you are trying to do with LocalContext, but the intent of LocalContext is to provide out-of-band storage for ambient values that need to flow across the data portal.

The utility and value of the context dictionaries has gone down a lot, especially since most of those out-of-band values today probably should be a Claim in the user's ClaimsIdentity. I'm sure there are other scenarios as well, but most scenarios I'm aware of now fit better into the use of the ClaimsIdentity.

That's why I want to understand what you are actually doing, and whether there's some other solution that fits into modern .NET better.

[mirecg]

I'm using it as an general context storage of values/objects which are available to anyone down in the call hiearchy and isolated between threads.

With LocalContext stored as AsyncLocal<ContextDictionary>, I cannot guarantee neither task isolation nor thread isolation.

[mirecg]

That's sad to hear.
I've provided enough examples with proof how easily can lead to race condition when writing to a LocalContext and even can lead to unpredictable results and behavior when using LocalContext.

OK, never mind. I didn't expect this would be fully adopted in the foreseeable future, if at all, I will have to deal with it somehow in my own fork.

Thank you for your time.

[ossendorf-at-hoelscher]

I'm not sure why you expect the LocalContext to be different, when you use the same application context. Maybe a solution for you could be using new DI scopes within your Parallel.ForEach()? But that would bring other problems like: Business objects becoming invalid after the new scope is disposed, etc.

[rockfordlhotka]

I've provided enough examples with proof how easily can lead to race condition when writing to a LocalContext and even can lead to unpredictable results and behavior when using LocalContext.

I agree. As I've said a couple times now, LocalContext has never been thread safe in the history of CSLA.

[mirecg]

As I've said a couple times now, LocalContext has never been thread safe in the history of CSLA.

I have to disagree. LocalContext is threadsafe if it is stored in TLS.

[StefanOssendorf]

As far as I'm concerned I think the stuff you described currently is the behvior I would expect using the LocalContext and not getting different isolated "bags".
Couldn't you just achieve what you want when you initialize every key you need before the Parallel.ForEach with TLS/AsyncLocal and put some extensions around that to make the unwrapping easier?

[Bowman74]

@mirecg
I think we are back to you creating your own implementation of IContextManager. That's what controls the instantiation of the local and client context managers. You can do whatever you want to make the context managers tied to a particular thread id.

Having said that, I'd be very careful with your assumption that having the context managers stored in TLS made them thread safe. That is very much not true. Outside of the obvious observation that it doesn't support threads interacting with the same storage at all, multiple current interrupting and even asynchronous operations using things like IO completion ports can all be executed on the same thread.

[rockfordlhotka]

The HybridDictionary was never threadsafe, but was never shared across threads because it was cloned.

Primitive values in the dictionary would have been threadsafe, because a copy was created for each thread by the CSLA BackgroundWorker or by the app's own code that did the clone when spinning off a thread.

Complex types in the dictionary might have been threadsafe. I honestly don't remember how we used to clone the dictionaries - and if we used BinaryFormatter it might have been safe. If we just copied the values, then complex types would have copied a reference and that would have not been threadsafe.

These days, LocalProxy does clone the dictionaries using MobileFormatter, so any logical server-side operations are entirely isolated from logical client-side operations. In that regard they are quite safe because the logical server is working with a copy of primitive and complex types.

Again, @mirecg, the behavior from "the old days" is still entirely possible to maintain in WinForms/WPF/Console apps - just use the TLS application context provider and clone the dictionaries any time you spin up a new thread. Exactly the way it has always been.

[mirecg]

The HybridDictionary was never threadsafe

agree, but that's not what I'm talking about

but was never shared across threads because it was cloned.

that's not true, it was not cloned, but TLS provides thread isolation for data stored in it.
LocalContext in TLS does not clone anything, unless somene clone it and pass it's instance to other threads explicitly.
When the LocalContext is obtained on the thread for the first time, it's null, so ContextManager will create it.
Each time new thread is created and someone access LocalContext for the first time on that thread, it will be created new instance of it.
So, each thread has it's own instance, so that code executing in different threads uses different instance of LocalContext, so that's why it is thread safe.

Again, @mirecg, the behavior from "the old days" is still entirely possible to maintain in WinForms/WPF/Console apps - just use the TLS application context provider and clone the dictionaries any time you spin up a new thread. Exactly the way it has always been.

I don't want to and I'm not able to stay in "old days" neither. That's why I've opend this discussion, because in "these days" there's mix of sync and async programming styles and I want to ensure that my code will work well in both cases.

In all respect to you, allow me to express my point of view. The word "Local" in LocalContext was in that "old days" local to thread. Nowadays, the "Local" in LocalContext should mean local per Task, but that's not true as can see it in those many examples I've provided.

Couldn't you just achieve what you want when you initialize every key you need before the Parallel.ForEach with TLS/AsyncLocal and put some extensions around that to make the unwrapping easier?

@StefanOssendorf The examples are provided in such way so that the problem is obvious. This it not real life example. In real life, you don't want to assume that someone up in the call hiearchy have prepared something in the context.

So again, let me repeat how I use it.
I'm using it in a general way to pass "contextual" data to code without passing it explicitly in every call down in the call hiearchy. It think this is general usage not just my own.
So for example you want to put correlationId in the context, you store it under known key "correlationId" so that every code "below" can access it without knowing who set it and can use it in for example logs or pass it further in some rest api call or so on.

Back to my code, again this is just to demonstrate problem

private static void Test_SameLocalContextInDifferentTasks(ApplicationContext applicationContext)
{
    // this will force to create instance of ContextDictionary for the current task
    var localContext = applicationContext.LocalContext;

    // simulate some work in parallel
    Parallel.ForEach(Enumerable.Range(0, 10), _ =>
    {
        var localContextInOtherTask = applicationContext.LocalContext;

        // the output will be True for all tasks,
        // because the LocalContext value is propagated from the invoking task due to AsyncLocal<T> behavior
        Console.WriteLine(Object.ReferenceEquals(localContextInOtherTask, localContext));

        try
        {
            // set correlationId for this execution, so that the code executing in this context will have it available
            // - this will be shared among all tasks, which will break the correlation logic
            applicationContext.LocalContext["correlationId"] = Guid.NewGuid();
        
            // do some work
            // - it will try to get the correlationId from LocalContext, for example to include it in the log,
            //   but it will obtain different value than it was set in this task
            DoSomeWork();
        }
        finally
        {
            // remove it from context - it is not needed anymore
            // the same problem here - LocalContext is shared among all tasks, so it will case race condition
            applicationContext.LocalContext.Remove("correlationId");
        }
    });
}

[ossendorf-at-hoelscher]

@mirecg I understand your point and purpose. Regarding "Local". I did never assume with "Local" is a thread referenced. I interpret "Local" as "This context is only available at this exeuction location." Where as the location can be either Client or Server and does not travel with the rest.
In that regard I would've never thought of your usage :D. But I never used CSLA prior to Version 7. I think your best bet is to create something like an extension method maybe which uses a TLS under the hood? That's just thinking loud because I have no idea if it will work. AsyncLocal is still some kind of magic to me.

[rockfordlhotka]

@mirecg There was never a design intent for the Global/Local/Client context values to be thread safe.

I am not aware of any magic that would have made a complex type thread safe just because it was referenced from a dictionary that was referenced from an item in TLS either. That seems like something entirely beyond what Microsoft would have implemented in .NET.

It has always been the case that, if you spin work onto a background thread, you need to clone the dictionaries onto that background thread.

When GlobalContext still existed there would have been an expectation of merging its changes back onto the main thread, and this is the primary reason GlobalContext was removed from CSLA when TPL and async/await became common. I wasn't interested in figuring out the merge conflict resolution issue.

In your Parallel.ForEach example, the solution is to clone the dictionaries onto each thread.

I really thought we had something like that at one point, but it isn't in the framework today.

The answer in my view is NOT to make the dictionaries thread safe - especially when you consider complex objects - that's a far reaching request, given that any serializable type can be put into Local/Client context, and in CSLA 9 that can include pretty arbitrary types thanks to custom serializers. We have no way of making CSLA business types thread safe, much less arbitrary other types.

The answer in my view is to help common scenarios create clones of the dictionaries on any background threads.

Pragmatically then, for your example, some helper so Parallel.ForEach can easily get an isolated context for each task is what makes the most sense to me.

Thankfully, ApplicationContext is no longer static, but is an instance, and each instance can have its own context manager type. What this means, is that regardless of the host's context manager (which is dependent on the .NET runtime), it should be possible to replace your

var localContextInOtherTask = applicationContext.LocalContext;

line with something that creates a Task-level ApplicationContext instance that has isolated context dictionaries. Very similar to what LocalProxy does actually.

csla/Source/Csla/DataPortalClient/LocalProxy.cs

Line 44 in 62015d1

private void InitializeContext(out IDataPortalServer _portal, out IServiceScope _logicalServerScope, out ApplicationContext _logicalServerApplicationContext)

If we could always assume the use of a DI scope based runtime this would be extremely easy, and I actually think, for the purposes of the Task example, using a DI scope based solution is probably correct - regardless of the host's runtime.

Basically, at the top of the Task you'd call a method like LocalProxy.InitializeContext, and then inside the task you'd make sure to always use the ApplicationContext created for the task.

The only complexity here, is that business objects are associated with a specific ApplicationContext. Specifically the one they are in when created. If you dig into LocalProxy, the business object graph gets "transferred" to the DI scoped context, then the data portal server runs, and the resulting object graph is "transferred" back to the calling DI context.

That same thing would need to happen for any domain objects accessed in the task as well.

That is problematic, because I'm guessing you want to use Parallel.ForEach to loop through child objects in a collection, and the way an object graph maintains its current ApplicationContext is at the root of the graph. In other words, child objects look to their parent (via the protected Parent property) to find the root of the graph, which maintains the current ApplicationContext.

So there's not a good way I can think of off hand to isolate specific objects in a graph onto different ApplicationContext s.

In short, Parallel.ForEach - with what I'm suggesting about an ApplicationContext helper - should work fine on a collection of root level objects, but I really don't see how it could safely operate on child items inside a single object graph.