fix!: add rw locks to client/api, hook accessor name

[toddbaert]

This PR adds multi-read/single-write locks to both the global singleton and the client. Particularly it addresses problems relating to the providers/hooks being mutated while an evaluation is occurring. This is especially important since some providers have provider hooks, and if we dont lock, it's possible hooks from proivder1 run on provider2 if an interleaving thread switched the provider.

To make things cleaner and less error-prone, I've created a little wrapper so we can use try-with-resources blocks.

This way, locking can be done like:

    public void addHooks(Hook... hooks) {
        try (AutoCloseableLock __ = rwLock.writeLockAutoCloseable()) {
            this.apiHooks.addAll(Arrays.asList(hooks));
        }
    }

...with no need to explicitly close the lock.

I'm going to write some tests in the morning for this before I mark it as ready.

Closes: #64

[codecov-commenter]

Codecov Report

Merging #131 (bf96c76) into main (71a5699) will increase coverage by 0.39%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main     #131      +/-   ##
============================================
+ Coverage     91.21%   91.60%   +0.39%     
- Complexity      177      187      +10     
============================================
  Files            19       20       +1     
  Lines           387      417      +30     
  Branches         23       22       -1     
============================================
+ Hits            353      382      +29     
  Misses           23       23              
- Partials         11       12       +1     

Flag	Coverage Δ
unittests	91.60% <100.00%> (+0.39%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
.../main/java/dev/openfeature/sdk/OpenFeatureAPI.java	100.00% <100.00%> (ø)
...in/java/dev/openfeature/sdk/OpenFeatureClient.java	97.91% <100.00%> (+0.26%)	⬆️
.../internal/AutoCloseableReentrantReadWriteLock.java	100.00% <100.00%> (ø)
src/main/java/dev/openfeature/sdk/HookSupport.java	95.12% <0.00%> (-2.44%)	⬇️

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

[toddbaert]

This is a more performant singleton. The synchronized block in the previous implementation involved inherent locking on each call. The "double check" singleton implementation can help here, but it also has it's downfalls.

I've used the "Bill Pugh" approach here.

This takes advantage of basic Java spec requirements that guarantee classes are loaded only once. There's no synchronization concerns here and no performance penalty.

[toddbaert]

I don't think we want anyone constructing this?

[justinabrahms]

Looks good so far. Interested to see the tests.

[toddbaert]

I almost have some tests working - but it's Canadian Thanksgiving and I have other obligations! I will pickup next week.

This is the tough part: open-feature/go-sdk#93 (comment)

[toddbaert]

This isn't directly related to my change, but just addresses a nasty testing issue.

Because we have a global singleton, many tests could interfere with eachother if they interleave. For example, 1 test might run api.clearHooks() immediately after another one does a setup that adds a hook.

This change runs every suite in a forked JVM, which adds a bit of startup time, but gives them each their own classloader to prevent interference via the singletons. I added a new test that happened to call api.clarHooks() and found it failed about 10% of the time without this fix.

[toddbaert]

Updated the grammar error in this XML comment

[toddbaert]

There wasn't a reasonable means of fully testing the locking with blackbox testing, so I've added this whitebox-style testing.

[toddbaert]

@justinabrahms I've added tests.

[toddbaert]

@justinabrahms @agentgonzo

I think I'm finished with this now. It's analogous to open-feature/dotnet-sdk#79 with a few java specific changes.

Initially I was locking in the evaluation, but I don't think that's necessary or beneficial. The current locks mainly protect from errors by guaranteeing ordering and preventing collection mutation during enumeration.

Additionally, I've made changes to access the provider just once in the evaluation logic. This prevents things from becoming incoherent if the provider is charged mid evaluation. I've added a specific test for this.

[toddbaert]

@justinabrahms one other thing worth pointing out ... I've changed the names of the accessors for hooks on the client and API, in order to be more consistent with addhooks, which is the same for both the API and client.

If you disagree with that, I can revert.

[justinabrahms]

@toddbaert Seems fine to me, but I changed your PR title to denote the breaking change.