[FEATURE] Ensure thread safety of sdk

[skyerus]

Requirements

Inspired by open-feature/dotnet-sdk#56

The global singletons (e.g. provider, evaluationContext, logger) all share a mutex to ensure thread safety, however a client doesn't use a mutex so there's a concern if two processes want to set an evaluation context for example.

The EvaluationContext type has the field Attributes of type map[string]interface{}. In go, maps are always passed by reference, this means that in theory one could pass an EvaluationContext to a client's flag evaluation call and continue to alter the Attributes' map in another process.
This could be avoided by making the Attributes field unexported with an exported constructor for EvaluationContext, this would mean that an application author couldn't change the field once constructed. However, an author could still alter the map they passed as a parameter to the constructor, which is the same map used in struct (passed by reference). In order to avoid this the constructor would need to initialise a new map and copy the map passed as a parameter.

Is this overkill? Is there a better solution we can come up with?

cc @beeme1mr @toddbaert @james-milligan

[toddbaert]

The global singletons (e.g. provider, evaluationContext, logger) all share a mutex to ensure thread safety, however a client doesn't use a mutex so there's a concern if two processes want to set an evaluation context for example.

This was my main concern (and still a concern in the Java SDK I think).

As for the evaluation context:

I think that a user mutating a map in multiple threads should be aware of the consequences of that. If we want to protect users from themselves, one other way could perhaps encapsulate the Attribues field and make the evaluation context a more robust struct with methods for adding values - this is similar to how the EvaluationContext works in Java. These methods could allow value addition in a threadsafe manner, using a mutex or some other threadsafe primitives behind the scenes.

I'm not sure if this solution is any better than yours. It does allow it to be modified after construction, but there's more code to maintain with my proposal I think.

This would of course be a breaking change. cc @beeme1mr

[james-milligan]

i personally lean more towards @toddbaert's approach, it feels cleaner to have an EvaluationContext type with a SetAttribute / SetAttributes method(s)

[beeme1mr]

If this is the approach, it must be a priority. To stay on schedule, this change would need to be made available tomorrow.

open-feature/spec#146

[toddbaert]

i personally lean more towards @toddbaert's approach, it feels cleaner to have an EvaluationContext type with a SetAttribute / SetAttributes method(s)

I kinda hate my approach but I worry that it might be the best one.

Although, I hadn't considered setAttribues (plural) as @james-milligan mentions where we could take an entire map. That's not really as simple in Java but doable in go I think. I think that would be quite convenient.

[skyerus]

Would allowing the the map to be modified after construction violate this spec requirement?

Requirement 4.1.4
The evaluation context MUST be mutable only within the before hook.

[toddbaert]

Would allowing the the map to be modified after construction violate this spec requirement?

Requirement 4.1.4
The evaluation context MUST be mutable only within the before hook.

That point is probably a bit misleading. We should improve the wording.

The actual point here is that only before hooks should return an evaluation context (one that's merged with the ambient context). The other hooks do not return the evaluation context. Whether mutating them is possible or not is more of a language and implementation concern.

[toddbaert]

I think you've just found another reason why we should think about thread safety though @skyerus 😅

[skyerus]

If other hooks have as much control over mutating the EvaluationContext as the Before hook does then it raises the question as to whether it remains a property specific to the Before hook 🤔

[toddbaert]

If other hooks have as much control over mutating the EvaluationContext as the Before hook does then it raises the question as to whether it remains a property specific to the Before hook thinking

I get your point, and you're right. The point should be changed to say something that indicates that only the before hook will merge a RETURNED context. Side effects mutating the referenced context in any hook are probably beyond the ability of the spec to dictate. I think there's too many variables across languages for us to reasonably police that. The fact before hooks allow authors to return a context makes it very explicit that using the before hooks to change context is safe and valid, since the SDK will carefully merge it for you.

[skyerus]

If this is the approach, it must be a priority. To stay on schedule, this change would need to be made available tomorrow.

open-feature/spec#146

I'm happy to drop what I'm currently working on and to pick this up now if we're happy with this approach.

[toddbaert]

I would love to have @kinyoklion 's opinion, since he was the genesis of this discussion... unfortunately he is on the West Coast, I think. I suppose if you start with a basic POC of the idea it would at least give us some data on if it feels right and solves our issues.

I vote you at start on that with an "experimental" mindset for now, until we get feedback from @kinyoklion and perhaps @justinabrahms .

cc @beeme1mr @benjiro

[skyerus]

type EvaluationContext struct {
   mx *sync.Mutex
   targetingKey string
   attributes map[string]interface{}
}

func (e *EvaluationContext) SetAttribute(key string, value interface{}) {
   e.mx.Lock()
   ...
}

func (e *EvaluationContext) SetAttributes(attrs map[string]interface{}) {
   e.mx.Lock()
   ...
}

func (e *EvaluationContext) SetTargetingKey(key string) {
   e.mx.Lock()
   ...
}

func NewEvaluationContext(targetingKey string, attributes map[string]interface{}) EvaluationContext {
     // copy attributes to new map to avoid reference being externally available
}

Loosely I'm thinking of something like this, does this align with what you had in mind?

[toddbaert]

yep! I just hope I haven't missed anything silly.

[james-milligan]

looks good to me

[justinabrahms]

In the javasdk, I'm leaning towards immutable eval context objects. Mutable state is just a headache and I'd love to avoid the complexity (at the expense of a few extra objects) if we can.

[toddbaert]

In the javasdk, I'm leaning towards immutable eval context objects. Mutable state is just a headache and I'd love to avoid the complexity (at the expense of a few extra objects) if we can.

I think this is a valid solution. And it's basically analogous to what @skyerus proposed.... but it's a significant and breaking change, right? We would have to remove all the add methods?

[kinyoklion]

I agree with @justinabrahms, when possible immutable objects are a better solution. I think a locking solution would work, but I personally would only use it after exhausting other options. It will protect from fundamental threading issues, but not logic/consistency issues around how an application developer may expect the system to behave.

It is preferable that once the context is produced it is either decoupled from the application developer entirely (as in a copy), or that it is immutable.

[toddbaert]

Like I said before:

I kinda hate my approach but I worry that it might be the best one.

I'd like to see what the immutable evaluation context API would look like in Java and Dotnet. I'm in favor of it if it can be done simply!

[skyerus]

In go perspective we can have a constructor (as depicted above) and omit the exported setters, this would enforce immutability (albeit somewhat uncomfortably so).
I'm also in the immutable is preferable camp, it reduces the surface area for issues imo.

[skyerus]

Created a POC PR with the changes we initially decided upon, I can remove the setters if we come to a decision on the immutability.

[toddbaert]

One idea that might allow us to defer this issue for a while:

At least in Java and Dotnet, we could extract the read/merge functionality of EvaluationContext into an interface, and then have a few implementations... some that are mutable and not threadsafe, some that are mutable and threadsafe, some that are immutable, etc. We wouldn't need to implement them all now. This will be compatible with whatever solutions we want to use in the future, and allow application authors to make their own decisions about thread safety and performance.

For now, it would be a simple change... we'd just create an interface (maybe called EvaluationContext and then create an implementation (MutableEvaluationContext, which would probably just be the current EvaluationContext).

This would allow us to differ this issue while preserving flexibility.

What do you guys think @kinyoklion @justinabrahms ?

[toddbaert]

@justinabrahms @kinyoklion @skyerus @james-milligan Here's an example of what I described above. We could even do this in Go, I think. If we wanted.

open-feature/java-sdk#112

[kinyoklion]

@justinabrahms @kinyoklion @skyerus @james-milligan Here's an example of what I described above. We could even do this in Go, I think. If we wanted.

open-feature/java-sdk#112

I will take a look. I also have a POC for using builders in dotnet:
open-feature/dotnet-sdk#77

[benjiro]

I'm leaning with @kinyoklion and @justinabrahms on making the EC immutable. Using mutexes can slow down the feature flag evaluation considerably under heavy concurrency(Think a high throughput web API). The more elegant solution would be to avoid mutex locking altogether by ensuring the EC is immutable.

What's the benefit of making the EC an interface other than buying time? I think we should only have a concert implementation of the EvalutionContext that is sealed(can't be extended). Seems like an unnecessary complexity with little benefit. Thoughts?

[justinabrahms]

@benjiro It buys us time and also forwards compatibility. If we decide that immutability makes things painful or generates too much GC thrash, we can release an alternative implementation without having to break our API contracts.

I know this isn't a concern in go bc go has awesome interfaces.. but some of us use java. :)

[skyerus]

A drawback to the interface solution is that it could be confusing to application authors using the client's API for the first time and see EvaluationContext as an interface. It's not immediately obvious what should be placed there, which makes it imo less friendly than being able to input a concrete struct. Perhaps this tradeoff is worth it for the sake of forwards compatibility.

[skyerus]

@toddbaert I've created an implementation in go similar to your java implementation

[toddbaert]

Looking at the code samples, and how the various opinions on this have shaken out, I propose that each SDK tackle this in their preferred way. This seems like the path of least resistance, since contributors to each particular language seem to agree with a direction. The spec is intentionally not prescriptive about this and it makes sense that implementing languages might make their own stylistic choices here (thread safety isn't even a thing in some languages).

• It sounds like @benjiro and @kinyoklion are in agreement on dotnet using an immutable context
• I won't speak for @justinabrahms , but I gleaned that he is OK with my interface proposal for some forward compatibility in Java...
• @skyerus seems to favor an immutable context, perhaps he and @james-milligan can decide what they prefer for Go
• JavaScript: what even is a thread??

[james-milligan]

@skyerus my vote is to take the immutability approach, i agree that using an interface may be confusing for developers

[c4milo]

@toddbaert, what was the conclusion for the Go SDK? immutability through #91?

[toddbaert]

@toddbaert, what was the conclusion for the Go SDK? immutability through #91?

In short, yes. If you have any additional concerns though, please don't hesitate to open a new issue.