Review thread safety of SDK about dotnet-sdk HOT 4 CLOSED

@kinyoklion @toddbaert are we happy to close out this issue now? I believe the main threading concerns have been resolved thanks to @kinyoklion efforts.

from dotnet-sdk.

Yep!

Closing with #79

from dotnet-sdk.

I don't have a strong proposal yet, but I can add a concrete example of where this is a problem.

The evaluation context that is passed to a client is not immutable, so it is possible to pass a reference to the client, and then continue to modify that reference.

This specific issue could be documented. Once an evaluation context has been passed to the client, then you cannot modify it any more. It could also be eliminated through having discrete builders that create immutable contexts, though at the cost of more code to maintain and some stricter rules to maintain. (Structured type data would need to be a value type after being built as well.)

I do think it would be best if there were at least some level of restriction or enforcement of immutability to prevent this. Documentation is great, but code is better, and it surfaces the issue much faster.

If you have a global context, and you are adding and removing things from it based on requests or other state, then it would be pretty easy to get into this situation.

If you modify the context, and concurrently do evaluations, then sometimes it will work, and sometimes it will not work. When it doesn't work you get something like this.

System.AggregateException: One or more errors occurred. (Collection was modified; enumeration operation may not execute.)
 ---> System.InvalidOperationException: Collection was modified; enumeration operation may not execute.
   at System.Collections.Generic.Dictionary`2.KeyCollection.Enumerator.MoveNext()
   at OpenFeature.SDK.Model.EvaluationContext.Merge(EvaluationContext other) in /home/rlamb/code/open-feature/dotnet-sdk/src/OpenFeature.SDK/Model/EvaluationContext.cs:line 223
   at OpenFeature.SDK.FeatureClient.EvaluateFlag[T](Func`4 resolveValueDelegate, FlagValueType flagValueType, String flagKey, T defaultValue, EvaluationContext context, FlagEvaluationOptions options) in /home/rlamb/code/open-feature/dotnet-sdk/src/OpenFeature.SDK/OpenFeatureClient.cs:line 222
   at OpenFeature.SDK.FeatureClient.GetBooleanDetails(String flagKey, Boolean defaultValue, EvaluationContext context, FlagEvaluationOptions config) in /home/rlamb/code/open-feature/dotnet-sdk/src/OpenFeature.SDK/OpenFeatureClient.cs:line 104

In this particular case the context merging was enumerating the context while I put a new field into the context.

I think that having some determinism here would be better. For instance always guaranteeing an exception in this scenario. One possible way to do that would be some form of soft immutability. Freezing the object is the first thing that comes to mind. Basically once the client took control of the EvaluationContext it could freeze it. Freezing would set an atomic internally to indicate it was frozen, and then any setter methods would throw some kind of exception. One complexity here it that we would also need to recursively freeze structure objects as they are subject to the same problem.

Aside form this I think there are some problems that may result in different results than expected, but I am not sure if other problems would cause crashes. Maybe setting a context and that context is not synchronized, so another thread doing a subsequent evaluation would be using a stale pointer, that type of problem.

from dotnet-sdk.

@kinyoklion thanks for this summary.

I think you're correct that we could solve this with doc, and also that code is better. I also think there are degrees of protection we can offer without adding too much of a burden on ourselves, and we need to find that balance. For example, even disregarding the client context, a user could instantiate a context, pass it to a thread X which mutates it, and also pass it to an evaluation in thread Y. This could cause the sort of issue you're referencing without even involving the client's member context. This is currently being discussed in the linked go-sdk issue.

Do we want to offer protection in this case too? Can we do that by using thread-safety primitives in the context itself?

I like your freezing proposal, but I think we should decide ahead of time how far we want to go in terms of thread safety assurances to application authors. I hope that makes sense!

from dotnet-sdk.