[FEATURE] Ensure thread safety of sdk about go-sdk HOT 31 CLOSED

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??

from go-sdk.

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.

from go-sdk.

@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

from go-sdk.

@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. :)

from go-sdk.

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

from go-sdk.

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.

from go-sdk.

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.

from go-sdk.

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

from go-sdk.

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.

from go-sdk.

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?

from go-sdk.

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?

from go-sdk.

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.

from go-sdk.

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.

from go-sdk.

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

from go-sdk.

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

from go-sdk.

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

from go-sdk.

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.

from go-sdk.

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 🤔

from go-sdk.

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.

from go-sdk.

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

from go-sdk.

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

from go-sdk.

looks good to me

from go-sdk.

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.

from go-sdk.

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!

from go-sdk.

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.

from go-sdk.

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 ?

from go-sdk.

@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

from go-sdk.

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?

from go-sdk.

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

from go-sdk.

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

from go-sdk.

@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.

from go-sdk.