Add setter for context #55
Conversation
Signed-off-by: Todd Baert <toddbaert@gmail.com>
Codecov Report
@@ Coverage Diff @@
## main #55 +/- ##
==========================================
+ Coverage 94.72% 94.73% +0.01%
==========================================
Files 18 18
Lines 417 418 +1
Branches 33 33
==========================================
+ Hits 395 396 +1
Misses 13 13
Partials 9 9
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| /// <summary> | ||
| /// Sets the EvaluationContext of the client<see cref="EvaluationContext"/> | ||
| /// </summary> | ||
| public void SetContext(EvaluationContext evaluationContext) => this._evaluationContext = evaluationContext; |
There was a problem hiding this comment.
It isn't specifically related to this change, but is this code generally expected to be thread safe?
There was a problem hiding this comment.
It isn't specifically related to this change, but is this code generally expected to be thread safe?
Hmm, there's nothing in the spec about it, but I think that we'd likely want the context in the client and the api singleton to be thread safe, so that we could have some predictability around it.
Are you thinking we should synchronize this method?
There was a problem hiding this comment.
I've not looked through the code much yet with an eye for thread safety. Synchronizing set/get access to this method would affect which instance is being used during an evaluation when it was concurrently modified, which would make ordering a bit more predictable.
That said the context itself is mutable, so thread behavior around mutating the context itself is likely more problematic.
I suspect there are currently opportunities for concurrent modification exceptions to be thrown if the context, or contained structures, was being mutated while other things may be trying to enumerate
Probably doesn't need to mingle with this PR.
There was a problem hiding this comment.
I will add an issue to investigate thread safety overall in the this and the Java SDK.
I suspect that whatever thread safety issues exist here we also have in Java.
That said the context itself is mutable, so thread behavior around mutating the context itself is likely more problematic.
I think mutating the client and global context is not really something we want to encourage, they should be get/set wholesale. Context can be mutated in hooks, where each hook has it's own context and where I believe there's no thread safety concerns. I hope that makes sense. I'm not sure what kind of guardrails C# can add for us here.
Adding this for consistency with other SDKs.