fixup: weakmap and throw

Signed-off-by: Todd Baert <todd.baert@dynatrace.com>

Author: toddbaert

src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java

@@ -6,10 +6,12 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.List;
+import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
+import java.util.WeakHashMap;
 import java.util.concurrent.ConcurrentLinkedQueue;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.Consumer;
@@ -38,9 +40,12 @@ public class OpenFeatureAPI implements EventBus<OpenFeatureAPI> {
      * Global registry tracking which API instance each provider is currently bound to.
      * Used to detect violations of spec requirement 1.8.4 (a provider SHOULD NOT be
      * registered with more than one API instance simultaneously).
+     *
+     * <p>Backed by a {@link WeakHashMap} so providers that are discarded without explicit
+     * deregistration do not pin the API instance in memory.
      */
-    private static final ConcurrentHashMap<FeatureProvider, OpenFeatureAPI> GLOBAL_PROVIDER_REGISTRY =
-            new ConcurrentHashMap<>();
+    private static final Map<FeatureProvider, OpenFeatureAPI> GLOBAL_PROVIDER_REGISTRY =
+            Collections.synchronizedMap(new WeakHashMap<>());
 
     // package-private multi-read/single-write lock (instance-level for isolation)
     final AutoCloseableReentrantReadWriteLock lock;
@@ -373,20 +378,23 @@ public void clearHooks() {
     }
 
     /**
-     * Registers a provider with the global registry, warning if it is already
+     * Registers a provider with the global registry, throwing if it is already
      * bound to a different API instance (spec requirement 1.8.4).
+     *
+     * @throws IllegalStateException if the provider is already bound to a different API instance
      */
     void registerGlobalProvider(FeatureProvider provider) {
-        GLOBAL_PROVIDER_REGISTRY.compute(provider, (p, existing) -> {
+        synchronized (GLOBAL_PROVIDER_REGISTRY) {
+            OpenFeatureAPI existing = GLOBAL_PROVIDER_REGISTRY.get(provider);
             if (existing != null && existing != this) {
-                log.warn("Provider "
+                throw new IllegalStateException("Provider "
                         + provider.getClass().getName()
                         + " is already registered with another API instance. "
                         + "A provider SHOULD NOT be bound to more than one API instance "
                         + "simultaneously (spec requirement 1.8.4).");
             }
-            return this;
-        });
+            GLOBAL_PROVIDER_REGISTRY.put(provider, this);
+        }
     }
 
     /**

src/main/java/dev/openfeature/sdk/ProviderRepository.java

@@ -170,11 +170,10 @@ private void prepareAndInitializeProvider(
             }
             FeatureProviderStateManager existing = getExistingStateManagerForProvider(newProvider);
             if (existing == null) {
+                openFeatureAPI.registerGlobalProvider(newProvider);
                 newStateManager = new FeatureProviderStateManager(newProvider);
                 // only run afterSet if new provider is not already attached
                 afterSet.accept(newProvider);
-                // spec 1.8.4: warn if this provider is already bound to another API instance
-                openFeatureAPI.registerGlobalProvider(newProvider);
             } else {
                 newStateManager = existing;
             }

src/test/java/dev/openfeature/sdk/GlobalProviderRegistryTest.java

@@ -1,63 +1,46 @@
 package dev.openfeature.sdk;
 
 import static org.assertj.core.api.Assertions.assertThatCode;
-import static org.mockito.ArgumentMatchers.contains;
-import static org.mockito.Mockito.never;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
 import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Test;
-import org.mockito.Mockito;
-import org.simplify4u.slf4jmock.LoggerMock;
-import org.slf4j.Logger;
 
 class GlobalProviderRegistryTest {
 
     /**
-     * Re-registering a provider with the same API instance should not produce a warning.
+     * Re-registering a provider with the same API instance should not throw.
      * This exercises the {@code existing == this} path in registerGlobalProvider.
      */
     @Test
-    @DisplayName("no warning when same API instance re-registers the same provider")
-    void noWarningWhenSameInstanceReRegisters() {
-        Logger mockLogger = Mockito.mock(Logger.class);
-        LoggerMock.setMock(OpenFeatureAPI.class, mockLogger);
-        try {
-            OpenFeatureAPI api = new OpenFeatureAPI();
-            NoOpProvider provider = new NoOpProvider();
-
-            api.registerGlobalProvider(provider);
-            api.registerGlobalProvider(provider); // same instance, second call
+    @DisplayName("no throw when same API instance re-registers the same provider")
+    void noThrowWhenSameInstanceReRegisters() {
+        OpenFeatureAPI api = new OpenFeatureAPI();
+        NoOpProvider provider = new NoOpProvider();
 
-            Mockito.verify(mockLogger, never()).warn(Mockito.anyString());
-        } finally {
-            LoggerMock.setMock(OpenFeatureAPI.class, null);
-        }
+        assertThatCode(() -> {
+                    api.registerGlobalProvider(provider);
+                    api.registerGlobalProvider(provider); // same instance, second call
+                })
+                .doesNotThrowAnyException();
     }
 
     /**
      * After deregistering a provider, binding it to a different API instance
-     * should not produce a warning — proving the entry was removed.
+     * should not throw, proving the entry was removed.
      */
     @Test
     @DisplayName("deregister removes provider from global registry")
     void deregisterRemovesProviderFromRegistry() {
-        Logger mockLogger = Mockito.mock(Logger.class);
-        LoggerMock.setMock(OpenFeatureAPI.class, mockLogger);
-        try {
-            OpenFeatureAPI api1 = new OpenFeatureAPI();
-            OpenFeatureAPI api2 = new OpenFeatureAPI();
-            NoOpProvider provider = new NoOpProvider();
-
-            api1.registerGlobalProvider(provider);
-            api1.deregisterGlobalProvider(provider);
+        OpenFeatureAPI api1 = new OpenFeatureAPI();
+        OpenFeatureAPI api2 = new OpenFeatureAPI();
+        NoOpProvider provider = new NoOpProvider();
 
-            // Should not warn because the provider was deregistered
-            api2.registerGlobalProvider(provider);
+        api1.registerGlobalProvider(provider);
+        api1.deregisterGlobalProvider(provider);
 
-            Mockito.verify(mockLogger, never()).warn(Mockito.anyString());
-        } finally {
-            LoggerMock.setMock(OpenFeatureAPI.class, null);
-        }
+        // should not throw because the provider was deregistered
+        assertThatCode(() -> api2.registerGlobalProvider(provider)).doesNotThrowAnyException();
     }
 
     /**
@@ -67,24 +50,19 @@ void deregisterRemovesProviderFromRegistry() {
     @Test
     @DisplayName("deregister is a no-op when called by non-owner instance")
     void deregisterIsNoOpForNonOwner() {
-        Logger mockLogger = Mockito.mock(Logger.class);
-        LoggerMock.setMock(OpenFeatureAPI.class, mockLogger);
-        try {
-            OpenFeatureAPI api1 = new OpenFeatureAPI();
-            OpenFeatureAPI api2 = new OpenFeatureAPI();
-            NoOpProvider provider = new NoOpProvider();
+        OpenFeatureAPI api1 = new OpenFeatureAPI();
+        OpenFeatureAPI api2 = new OpenFeatureAPI();
+        NoOpProvider provider = new NoOpProvider();
 
-            api1.registerGlobalProvider(provider);
+        api1.registerGlobalProvider(provider);
 
-            // api2 is not the owner — this should be a no-op
-            api2.deregisterGlobalProvider(provider);
+        // api2 is not the owner; this should be a no-op
+        api2.deregisterGlobalProvider(provider);
 
-            // api2 re-registering should still warn, because api1 still owns it
-            api2.registerGlobalProvider(provider);
-            Mockito.verify(mockLogger).warn(contains("1.8.4"));
-        } finally {
-            LoggerMock.setMock(OpenFeatureAPI.class, null);
-        }
+        // api2 re-registering should still throw, because api1 still owns it
+        assertThatThrownBy(() -> api2.registerGlobalProvider(provider))
+                .isInstanceOf(IllegalStateException.class)
+                .hasMessageContaining("1.8.4");
     }
 
     /**

src/test/java/dev/openfeature/sdk/isolated/IsolatedAPITest.java

@@ -1,7 +1,7 @@
 package dev.openfeature.sdk.isolated;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.mockito.ArgumentMatchers.contains;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
 import dev.openfeature.sdk.ImmutableContext;
 import dev.openfeature.sdk.NoOpProvider;
@@ -17,9 +17,6 @@
 import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.Timeout;
-import org.mockito.Mockito;
-import org.simplify4u.slf4jmock.LoggerMock;
-import org.slf4j.Logger;
 
 class IsolatedAPITest {
 
@@ -211,25 +208,20 @@ void clientUsesItsOwnInstanceProvider() throws Exception {
     }
 
     /**
-     * Requirement 1.8.4 — a warning is logged when the same provider instance
+     * Requirement 1.8.4 — an exception is thrown when the same provider instance
      * is registered with more than one API instance simultaneously.
      */
     @Test
-    @DisplayName("warn when same provider bound to multiple API instances (req 1.8.4)")
-    void warnWhenProviderBoundToMultipleInstances() {
-        Logger mockLogger = Mockito.mock(Logger.class);
-        LoggerMock.setMock(OpenFeatureAPI.class, mockLogger);
-        try {
-            OpenFeatureAPI api1 = OpenFeatureAPI.createIsolated();
-            OpenFeatureAPI api2 = OpenFeatureAPI.createIsolated();
-
-            NoOpProvider provider = new NoOpProvider();
-            api1.setProvider(provider);
-            api2.setProvider(provider);
-
-            Mockito.verify(mockLogger).warn(contains("1.8.4"));
-        } finally {
-            LoggerMock.setMock(OpenFeatureAPI.class, null);
-        }
+    @DisplayName("throw when same provider bound to multiple API instances (req 1.8.4)")
+    void throwWhenProviderBoundToMultipleInstances() {
+        OpenFeatureAPI api1 = OpenFeatureAPI.createIsolated();
+        OpenFeatureAPI api2 = OpenFeatureAPI.createIsolated();
+
+        NoOpProvider provider = new NoOpProvider();
+        api1.setProvider(provider);
+
+        assertThatThrownBy(() -> api2.setProvider(provider))
+                .isInstanceOf(IllegalStateException.class)
+                .hasMessageContaining("1.8.4");
     }
 }