Does the Context Need to be Cloned?

[nathanmascitelli]

Describe the bug The chanes in #178 have brought the allocations down but cloning the Context still shows up when I run dotnet trace against my application, this time around creating Dictionary<string, string>: Note that this is showing up as CPU usage as opposed to memory.

Looking at the code there is nothing obvious that can be improved, if we need to create a copy of the Context then the Properties dictionary must be recreated. However, I have to ask why do we need to create a copy of the Context? My understanding is that the Context is meant to be created and sent along with every request to Unleash so I don't think there is a concern for multiple threads interacting with the Properties dictionary.

Could we get away with not creating a copy of the provided Context and just use what's passed to us directly?

[daveleek]

Hello @nathanmascitelli!

My understanding is that the Context is meant to be created and sent along with every request to Unleash so I don't think there is a concern for multiple threads interacting with the Properties dictionary.

Multithreading on properties dictionary would be bad indeed, but using the same context for every request and not properly replacing userId and sessionid fields would also skew evaluations/results. I see the only fields being set on the context internally from configuration during evaluation are AppName and Environment. Those shouldn't change during a lifecycle of the app so I think it should be a safe thing to change.

There's a default implementation of context provider that creates one on instantiation and holds on to it forever, that would have to change if we're changing this behavior. That should be the only case of this in this code base though

[nathanmascitelli]

Looking at DefaultUnleashContextProvider I'm not sure we need to do anything. Its an internal implementation of IUnleashContextProvider that is set only if the user does not set their own IUnleashContextProvider. Its used internally only again if a context is not provided when checking flags or variants. So there is no opertunity for the user to modify the shared context. Sure AppName and Environment will be set but thats fine. I don't think we actually need to change anything.

As an asside, should we only set AppName and Environment if they have not already been set? Right now we will override user input.

@daveleek

[daveleek]

Looking at DefaultUnleashContextProvider I'm not sure we need to do anything. Its an internal implementation of IUnleashContextProvider that is set only if the user does not set their own IUnleashContextProvider. Its used internally only again if a context is not provided when checking flags or variants.

That's a good point @nathanmascitelli. The retrieved context is only used internally, so the properties are never touched.

As an asside, should we only set AppName and Environment if they have not already been set? Right now we will override user input.

I think that's what we're already doing, old code:

var environment = string.IsNullOrEmpty(Environment) ? settings.Environment : Environment;

New code in your PR:

Environment = string.IsNullOrEmpty(Environment) ? settings.Environment : Environment;

Both of these use settings.Environment if true ie this.Environment is null or empty

[nathanmascitelli]

I think that's what we're already doing

Wow yup I just had a total brain fart there...