feat!: Thread safe hooks, provider, and context #79

kinyoklion · 2022-10-07T22:13:57Z

Change storage type of global hooks to a concurrent stack.
- Should be able to add and remove from this without locking,
Protect references used from multiple threads with a Read/Write lock.
Change read only collection types to immutable collection types.
- Readonly collections are just views into existing mutable collections. So they are still subject to the same modification problems. Immutable collections are supposed to be fixed and unchanging. Considering they are an interface you could still violate this constraint, but it is less likely.

Related to: #56

Signed-off-by: Ryan Lamb <[email protected]>

…he one at the time of client instantiation. Signed-off-by: Ryan Lamb <[email protected]>

codecov-commenter · 2022-10-07T22:16:49Z

Codecov Report

Merging #79 (094f3d2) into main (d980a94) will increase coverage by 0.44%.
The diff coverage is 97.80%.

@@            Coverage Diff             @@
##             main      #79      +/-   ##
==========================================
+ Coverage   94.08%   94.52%   +0.44%     
==========================================
  Files          20       20              
  Lines         473      530      +57     
  Branches       36       37       +1     
==========================================
+ Hits          445      501      +56     
  Misses         16       16              
- Partials       12       13       +1

Impacted Files	Coverage Δ
src/OpenFeatureSDK/Model/FlagEvaluationOptions.cs	`91.66% <85.71%> (ø)`
src/OpenFeatureSDK/OpenFeatureClient.cs	`98.74% <97.56%> (-0.52%)`	⬇️
src/OpenFeatureSDK/FeatureProvider.cs	`100.00% <100.00%> (ø)`
src/OpenFeatureSDK/OpenFeature.cs	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Signed-off-by: Ryan Lamb <[email protected]>

kinyoklion · 2022-10-07T22:38:02Z

src/OpenFeatureSDK/OpenFeature.cs

-        /// <returns>A immutable list of <see cref="Hook"/></returns>
-        public IReadOnlyList<Hook> GetHooks() => this._hooks.AsReadOnly();
+        /// <returns>Enumeration of <see cref="Hook"/></returns>
+        public IEnumerable<Hook> GetHooks() => this._hooks.Reverse();


In the open feature API and Client, the SDK controls the implementation type and can promise the use of a concurrent collection.

In the provider this is not the case, so that is constrained to an immutable collection.

Also, I am reversing these for now. I wasn't completely comfortable with all the test changes required for them to be in the opposite order.

Signed-off-by: Ryan Lamb <[email protected]>

kinyoklion · 2022-10-07T22:46:57Z

src/OpenFeatureSDK/FeatureProvider.cs

-        /// <returns></returns>
-        public virtual IReadOnlyList<Hook> GetProviderHooks() => Array.Empty<Hook>();
+        /// <returns>Immutable list of hooks</returns>
+        public virtual IImmutableList<Hook> GetProviderHooks() => ImmutableList<Hook>.Empty;


Alternatively this could be enumerable like other places, with documentation that the provider must use a thread-safe implementation. Technically a readonlylist, with the same constraint, would also works. I think immutable directs people in a way that the easiest solution is also the correct solution.

I think immutable directs people in a way that the easiest solution is also the correct solution.

Agreed.

kinyoklion · 2022-10-07T22:51:03Z

src/OpenFeatureSDK/OpenFeatureClient.cs

                .Concat(options?.Hooks ?? Enumerable.Empty<Hook>())
-                .Concat(this._featureProvider.GetProviderHooks())
+                .Concat(OpenFeature.Instance.GetProvider().GetProviderHooks())


Ok. This isn't correct. You would't want the hooks from a different provider than the evaluation method. Those two things need to be resolved at the same time.

I think that we need to get the reference, then use that reference to get the method and the hooks. We would not just want to get them in the call, because they could still be separated in time.

Signed-off-by: Ryan Lamb <[email protected]>

src/OpenFeatureSDK/OpenFeatureClient.cs

Signed-off-by: Ryan Lamb <[email protected]>

kinyoklion · 2022-10-10T17:14:19Z

src/OpenFeatureSDK/OpenFeature.cs

+        public void SetProvider(FeatureProvider featureProvider)
+        {
+            this._featureProviderLock.EnterWriteLock();
+            try


None of these code blocks can throw, but when using this type of lock I think using try/finally is a customary way to release the lock.

Yep, agreed. In fact, I made a little lock-wrapper class that allows us to use a try-with-resources block in Java (I think c# has an equivalent with the using keyword and IDisposable implementations).

Yeah, it could be better to make a couple little RAII objects for locks.

I am not sure for a couple reasons. The first is that it would generate an extra bit of garbage for each access to these fields. It would likely be negligible.

The second is that all the locking is currently only exposed in this one file.

If we were to need to expose the locking outside this file, then I could see the idea of returning a read lock protected object which the consumer could use in a using block.

Especially for double locks, where they could nest the using statement.

Signed-off-by: Ryan Lamb <[email protected]>

kinyoklion · 2022-10-10T17:25:07Z

src/OpenFeatureSDK/OpenFeature.cs

+        /// operations. For instance, during an evaluation the flag resolution method, and the provider hooks
+        /// should be accessed from the same reference, not two independent calls to
+        /// <see cref="OpenFeature.GetProvider"/>.
+        /// </para>


One thing I am not calling out here is that setting hooks, provider, and context are not guaranteed to be ordered among themselves. If you have one thread setting all 3, and another thread doing evaluations, then a single evaluation may use some combination of the values being set.

One thing I am not calling out here is that setting hooks, provider, and context are not guaranteed to be ordered among themselves. If you have one thread setting all 3, and another thread doing evaluations, then a single evaluation may use some combination of the values being set.

In Java and go, we read lock as soon as the evaluation starts while we get the provider, the context, and the hooks. I think this might be worth doing.

As for the block comment, I had added something similar in the read-lock block in Java, so it makes sense to me and I think it's worth pointing out.

So, here each of the 3 things are individually thread safe.

The provider will use a stable reference, which covers the provider hooks and other data.

The context is immutable, and we get a reference at a specific point in time. The reference globally may change, but we are decoupled from that because we got a reference. Locking for the duration of getting the context and the provider wouldn't prevent interleaved setting of them. You could set one, and then the evaluation gets the lock, you have to wait, and then set the other.

The last bit are the other two sets of hooks. These are interesting. There isn't anything which inherently couples them to anything else. They are copied, so the list will be stable in the places they are iterate, but up until they are iterated, they could change.

If you have cross dependent hooks then they should be added with AddHooks, which will add them atomically. I should document that.

I added a couple comments to the hook methods.

If you have cross dependent hooks

Worth saying I generally think this should be avoided, but I think people might do it.

Locking for the duration of getting the context and the provider wouldn't prevent interleaved setting of them.

Wouldn't read-locking in the evaluation on a shared lock which is write-locked by both the provider and context mutators address exactly this? Or am I misunderstanding?

Not unless you also locked for the duration of setting both of them.

If they can be independently set, as they can in the implementation at the moment, then it doesn't.

Thread 1: SetProvider(provider) writeLock(_provider) // Evaluation could start here SetContext(context) writeLock(_context) Thread 2: Evaluate() readLock(_provider) readLock(_context)

So, given this code someone could intend to set the provider and context, but it would be possible for evaluation to grab the lock before thread 1 set the context, but after it set the provider.

In order to not interleave it would need to be more like

Thread 1: writeLock(_provider) writeLock(_context) setProvider setContext Thread 2: Evaluate() readLock(_provider) readLock(_context)

Or use a single lock (but still set both)

Ahh, yes, I agree. I misunderstood your initial statement.

kinyoklion · 2022-10-10T17:29:34Z

src/OpenFeatureSDK/OpenFeatureClient.cs

@@ -241,7 +273,7 @@ public FeatureClient(FeatureProvider featureProvider, string name, string versio
                flagKey,
                defaultValue,
                flagValueType, this._metadata,
-                OpenFeature.Instance.GetProviderMetadata(),
+                provider.GetMetadata(),


We don't want to call GetProviderMetadata here on the OpenFeature singleton, because it could have change between when we got the resolution method and here. We want to be sure to only use the cached reference.

kinyoklion · 2022-10-10T17:34:26Z

src/OpenFeatureSDK/OpenFeatureClient.cs

        /// <param name="name">Name of client <see cref="ClientMetadata"/></param>
        /// <param name="version">Version of client <see cref="ClientMetadata"/></param>
        /// <param name="logger">Logger used by client</param>
        /// <param name="context">Context given to this client</param>
        /// <exception cref="ArgumentNullException">Throws if any of the required parameters are null</exception>
-        public FeatureClient(FeatureProvider featureProvider, string name, string version, ILogger logger = null, EvaluationContext context = null)


~~Just realized I need to re-introduce the equivalent of this check elsewhere.~~

I lifted the behavior the Java SDK has. Which is to log that it was not set, and then use the NoOp provider.

Signed-off-by: Ryan Lamb <[email protected]>

kinyoklion · 2022-10-10T18:02:18Z

src/OpenFeatureSDK/OpenFeatureClient.cs

+            if (provider == null)
+            {
+                provider = new NoOpFeatureProvider();
+                this._logger.LogDebug("No provider configured, using no-op provider");


This is debug in the Java SDK, but I wonder if maybe it should be more stern?

I think No-op is a valid configuration, and may be used intentionally, especially if OpenFeature is implemented in 3rd party libraries.

For this reason, I think this should remain a debug level log.

src/OpenFeatureSDK/OpenFeature.cs

toddbaert

Thanks @kinyoklion ! This looks great. Please consider my comments, though I'm not convinced any modifications are necessary. I think you've addressed the major concerns by using a consistent reference to provider during the course of evaluation.

Signed-off-by: Ryan Lamb <[email protected]>

kinyoklion · 2022-10-11T22:31:54Z

I realized that I missed the client level context locking. So I have added that.

kinyoklion · 2022-10-11T22:35:15Z

I've noticed another thing that needs to be resolved. The ReaderWriterLock type is disposable, so the containing type should implement disposable. Not really a problem for the global OpenFeature instance, because it isn't going anywhere. It is inconvenient for the client instances to be disposable though.

So I may want to consider a normal lock in that case.

benjiro

LGTM 👍🏻

Signed-off-by: Ryan Lamb <[email protected]>

kinyoklion · 2022-10-11T22:42:55Z

@toddbaert @benjiro I switched the OpenFeatureClient locks to normal locks. This does mean if you are using one client from many threads, that it will have more contention. Maybe... make more clients.

The alternative would be to make it disposable, but that then becomes like a, "what color are your functions" kind of situation.

In OpenFeature.cs I am not disposing. I added a comment. The singleton isn't meant to go away, so disposing it doesn't make sense.

toddbaert · 2022-10-12T01:17:46Z

@toddbaert @benjiro I switched the OpenFeatureClient locks to normal locks. This does mean if you are using one client from many threads, that it will have more contention. Maybe... make more clients.

Clients are lightweight, so this seems like a fine recommendation, we could even document it if you think it's worth it.

The alternative would be to make it disposable, but that then becomes like a, "what color are your functions" kind of situation.

Agreed (and I'll have to re-read that post now since it's been a while).

In OpenFeature.cs I am not disposing. I added a comment. The singleton isn't meant to go away, so disposing it doesn't make sense.

Yep, makes sense to me.

toddbaert · 2022-10-12T14:11:23Z

I know there's been a few changes on this, but I think it's this is good work and certainly more threadsafe than what we had before. I'm in favor of merging it unless there's objections.

🤖 I have created a release *beep* *boop* --- ## [0.4.0](v0.3.0...v0.4.0) (2022-10-12) ### ⚠ BREAKING CHANGES * Thread safe hooks, provider, and context (#79) * Implement builders and immutable contexts. (#77) ### Features * Implement builders and immutable contexts. ([#77](#77)) ([d980a94](d980a94)) * Thread safe hooks, provider, and context ([#79](#79)) ([609016f](609016f)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

kinyoklion added 2 commits October 7, 2022 14:34

Immutable hooks.

9e4ee6d

Signed-off-by: Ryan Lamb <[email protected]>

Make references volatile, always use the global provider instead of t…

08e68ba

…he one at the time of client instantiation. Signed-off-by: Ryan Lamb <[email protected]>

kinyoklion changed the title ~~Additional thread safety updates.~~ feat: Thread safe hooks, provider, and context Oct 7, 2022

Reverse hooks. Make hooks in FeatureClient a ConcurrentStack.

bd7ba04

Signed-off-by: Ryan Lamb <[email protected]>

kinyoklion commented Oct 7, 2022

View reviewed changes

Update documentation on GetHooks methods.

dda0f28

Signed-off-by: Ryan Lamb <[email protected]>

kinyoklion force-pushed the feat/thread-safety branch from d6d186b to dda0f28 Compare October 7, 2022 22:42

kinyoklion commented Oct 7, 2022

View reviewed changes

Use the same provider reference for the duration of an evaluation.

a86fb83

Signed-off-by: Ryan Lamb <[email protected]>

kinyoklion commented Oct 10, 2022

View reviewed changes

src/OpenFeatureSDK/OpenFeatureClient.cs Show resolved Hide resolved

kinyoklion added 2 commits October 10, 2022 10:12

Use slim read/write lock.

4a6736f

Signed-off-by: Ryan Lamb <[email protected]>

Update to use try/finally.

392a005

Signed-off-by: Ryan Lamb <[email protected]>

kinyoklion commented Oct 10, 2022

View reviewed changes

Add documentation.

b75e3e3

Signed-off-by: Ryan Lamb <[email protected]>

kinyoklion commented Oct 10, 2022

View reviewed changes

kinyoklion marked this pull request as ready for review October 10, 2022 17:31

kinyoklion requested review from benjiro and toddbaert October 10, 2022 17:31

kinyoklion changed the title ~~feat: Thread safe hooks, provider, and context~~ feat!: Thread safe hooks, provider, and context Oct 10, 2022

kinyoklion commented Oct 10, 2022

View reviewed changes

Use no-op provider when global provider is null.

c7f2cc3

Signed-off-by: Ryan Lamb <[email protected]>

kinyoklion commented Oct 10, 2022

View reviewed changes

toddbaert reviewed Oct 11, 2022

View reviewed changes

src/OpenFeatureSDK/OpenFeature.cs Show resolved Hide resolved

toddbaert reviewed Oct 11, 2022

View reviewed changes

src/OpenFeatureSDK/OpenFeature.cs Show resolved Hide resolved

toddbaert self-requested a review October 11, 2022 15:37

toddbaert approved these changes Oct 11, 2022

View reviewed changes

Add documentation on adding hooks.

f70d391

Signed-off-by: Ryan Lamb <[email protected]>

toddbaert mentioned this pull request Oct 11, 2022

fix!: add rw locks to client/api, hook accessor name open-feature/java-sdk#131

Merged

Add locking for client context.

51cf8f2

Signed-off-by: Ryan Lamb <[email protected]>

benjiro approved these changes Oct 11, 2022

View reviewed changes

kinyoklion added 2 commits October 11, 2022 15:39

Use normal locks for the client level.

b6658b6

Signed-off-by: Ryan Lamb <[email protected]>

Add comment about disposing.

094f3d2

Signed-off-by: Ryan Lamb <[email protected]>

kinyoklion merged commit 609016f into main Oct 12, 2022

kinyoklion deleted the feat/thread-safety branch October 12, 2022 18:27

github-actions bot mentioned this pull request Oct 12, 2022

chore(main): release 0.4.0 #78

Merged

toddbaert mentioned this pull request Oct 12, 2022

Review thread safety of SDK #56

Closed

This was referenced Jan 18, 2023

chore(main): release 1.0.0 #103

Closed

chore(main): release 1.0.0 #104

Closed

chore(main): release 1.0.0 #105

Closed

chore(main): release 1.0.0 #106

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Thread safe hooks, provider, and context #79

feat!: Thread safe hooks, provider, and context #79

kinyoklion commented Oct 7, 2022 •

edited

Loading

codecov-commenter commented Oct 7, 2022 •

edited

Loading

kinyoklion Oct 7, 2022

kinyoklion Oct 7, 2022

kinyoklion Oct 7, 2022

toddbaert Oct 11, 2022

kinyoklion Oct 7, 2022

kinyoklion Oct 7, 2022

kinyoklion Oct 10, 2022

toddbaert Oct 11, 2022

kinyoklion Oct 11, 2022

kinyoklion Oct 11, 2022

kinyoklion Oct 10, 2022

toddbaert Oct 11, 2022

kinyoklion Oct 11, 2022

kinyoklion Oct 11, 2022

toddbaert Oct 11, 2022

toddbaert Oct 11, 2022

kinyoklion Oct 11, 2022 •

edited

Loading

toddbaert Oct 11, 2022

kinyoklion Oct 10, 2022

kinyoklion Oct 10, 2022 •

edited

Loading

kinyoklion Oct 10, 2022

toddbaert Oct 11, 2022

toddbaert left a comment

kinyoklion commented Oct 11, 2022

kinyoklion commented Oct 11, 2022

benjiro left a comment

kinyoklion commented Oct 11, 2022

toddbaert commented Oct 12, 2022

toddbaert commented Oct 12, 2022

feat!: Thread safe hooks, provider, and context #79

feat!: Thread safe hooks, provider, and context #79

Conversation

kinyoklion commented Oct 7, 2022 • edited Loading

codecov-commenter commented Oct 7, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kinyoklion Oct 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kinyoklion Oct 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toddbaert left a comment

Choose a reason for hiding this comment

kinyoklion commented Oct 11, 2022

kinyoklion commented Oct 11, 2022

benjiro left a comment

Choose a reason for hiding this comment

kinyoklion commented Oct 11, 2022

toddbaert commented Oct 12, 2022

toddbaert commented Oct 12, 2022

kinyoklion commented Oct 7, 2022 •

edited

Loading

codecov-commenter commented Oct 7, 2022 •

edited

Loading

kinyoklion Oct 11, 2022 •

edited

Loading

kinyoklion Oct 10, 2022 •

edited

Loading