MarimerLLC / csla

A home for your business logic in any .NET application.
https://cslanet.com
MIT License
1.23k stars 394 forks source link

Switch out HybridDictionary for ConcurrentDictionary for Local/Client context #4015

Closed rockfordlhotka closed 3 weeks ago

rockfordlhotka commented 3 weeks ago

As per the conversation below, if we switch Local/Client context from the HybridDictionary base to ConcurrentDictionary, the collections themselves should be usable from multiple concurrent threads with some precautions.

To do this, the concrete type currently used for the LocalContext and ClientContext properties should be changed to an interface so we can preserve backward compatibility with existing code that uses those types.


Ah now I get what you mean. The used baseclass of ContextDictionary HybridDictionary is not thread safe. That's correct according to the documentation. We could probably move to a more thread safe implementation like you suggested. Since that would be a big breaking change we have to carefully consider that. @rockfordlhotka Your thoughts? My first thoughts on this are to create a new IContextDictionary interface which provides all methods provided by our current dictionary. We have to change the type used in the interfaces but at least we keep the api itself stable and could then make the actual implementation configurable while the current is the default.

Originally posted by @StefanOssendorf in https://github.com/MarimerLLC/csla/discussions/4012#discussioncomment-9712171

Bowman74 commented 3 weeks ago

@rockfordlhotka Just as a note, I could not create a unified IContextDictionary type, instead there is a different IClientContext and ILocalContext types. They look the same and ContextDictionary implements both of them. This was done for two reasons:

rockfordlhotka commented 3 weeks ago

You are now thinking to inject these into all the different application context manager types?

Until now, each of the different context managers has been responsible for creating and storing the dictionaries, but I suppose they could be considered to be a service - maybe?

There's some interesting stuff in LocalProxy when the proxy creates (sometimes) a new DI scope and copies (iirc) the Local/Client context values into the new DI scope. I think that's done with some sort of clone operation, but would have to look to be sure.

In that case, the new application context manager created for the temporary DI scope might not want to get new dictionaries, as it will need the clones.

StefanOssendorf commented 3 weeks ago

How is the creation of the actual context implementation now handled? We could resolve it from the DI. With .Net 8 (IIRC) we could even use the Keyed-DI-Registration feature but would have problems down the road? Or we only support that on .Net 8+? On the other hand we could create a configuration extension which enforces that the interface implementation has to provide a default constructor. With that we could safely create new instance during runtime with an empty ctor.

Tbh I have no clue how the context is used in the wild. We don't use it (yet?) but I can imagine people who want dependencies in there? But during writing this, the context itself is serialized and deserialized. So the dependencies would have to be re-populated in the new context. Mhm... I find my default constructor constraint more appealing writing this xD

rockfordlhotka commented 3 weeks ago

Kevin and I chatted and the DI thing is scope creep that seems unnecessary. This whole concept hasn't changed substantially in 20 years, and I expect it won't for another 20, so there's not a real need for adding abstractions just for fun.

Mostly people use ClientContext to store things that probably should (now days) be a claim in the user identity - like the current department/division or other things that affect the behavior of the UI or rules.

LocalContext is sometimes used to flow temporary "out of band" info, often during server-side data portal operations. Like the current database to be used and things like that.

Really, there are alternatives to these context dictionaries, especially now with DI, so I'm not sure they are as important as they once were.

Bowman74 commented 3 weeks ago

@rockfordlhotka, @StefanOssendorf, OK, there is a PR in for this. A couple of thoughts.