From 33e9d92e04fea1f10537d662657dfbbefd8e85da Mon Sep 17 00:00:00 2001 From: "Jonathan S. Fisher" Date: Sat, 16 Aug 2025 10:39:02 -0500 Subject: [PATCH 1/2] Fix #3 - Live Lock. Custom scopes that depend on custom scopes can sometimes create a 'Rescursive Update' problem depending on thread ordering. Fix using a CompletableFuture and atomic map operations. --- .../org/tomitribe/microscoped/core/Scope.java | 82 ++++++++++++++++--- 1 file changed, 69 insertions(+), 13 deletions(-) diff --git a/microscoped-core/src/main/java/org/tomitribe/microscoped/core/Scope.java b/microscoped-core/src/main/java/org/tomitribe/microscoped/core/Scope.java index eebca4c..7db8dda 100644 --- a/microscoped-core/src/main/java/org/tomitribe/microscoped/core/Scope.java +++ b/microscoped-core/src/main/java/org/tomitribe/microscoped/core/Scope.java @@ -16,16 +16,16 @@ */ package org.tomitribe.microscoped.core; -import javax.enterprise.context.spi.Contextual; -import javax.enterprise.context.spi.CreationalContext; import java.util.Map; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CompletionException; import java.util.concurrent.ConcurrentHashMap; -class Scope { - private final Instance NOTHING = new Instance<>(null, null, null); - - private final Map, Instance> instances = new ConcurrentHashMap<>(); +import javax.enterprise.context.spi.Contextual; +import javax.enterprise.context.spi.CreationalContext; +class Scope { + private final Map, CompletableFuture> instances = new ConcurrentHashMap<>(); private final Key key; public Scope(final Key key) { @@ -43,7 +43,30 @@ public Key getKey() { * @return existing or newly created bean instance, never null */ public T get(final Contextual contextual, final CreationalContext creationalContext) { - return (T) instances.computeIfAbsent(contextual, c -> new Instance<>(contextual, creationalContext)).get(); + CompletableFuture future = instances.get(contextual); + if (future == null) { + final CompletableFuture newFuture = new CompletableFuture<>(); + // Atomically place the new future in the map. + future = instances.putIfAbsent(contextual, newFuture); + if (future == null) { + // We won the race. Our future is now in the map. We are responsible for creating the bean. + future = newFuture; + try { + // The creation logic is now safely executed by only one thread. + final Instance createdInstance = new Instance<>(contextual, creationalContext); + future.complete(createdInstance); // Publish the result for other threads. + } catch (final Throwable e) { + // If creation fails, complete the future exceptionally and remove it from the map + // so that subsequent requests can try again. + future.completeExceptionally(e); + instances.remove(contextual, future); + // Re-throw the original exception. + throw e; + } + } + } + // All threads (the winner and the waiters) wait here for the result. + return fastWaitForValue(future); } /** @@ -53,7 +76,35 @@ public T get(final Contextual contextual, final CreationalContext crea * @return existing the bean instance or null */ public T get(final Contextual contextual) { - return (T) instances.getOrDefault(contextual, NOTHING).get(); + final CompletableFuture future = instances.get(contextual); + return fastWaitForValue(future); + } + + private static T fastWaitForValue(final CompletableFuture future) throws Error { + final T value; + if (future == null) { + value = null; + } else if (future.isDone() && !future.isCompletedExceptionally()) { + // Completed normally: read the result without join() + final Instance instance = future.getNow(null); // never null in this branch + value = instance.get(); + } else { + try { + final Instance instance = future.join(); + value = instance.get(); + } catch (final CompletionException ce) { + // Try to hide internal semantics here in case someone is expecting a CreationException or something + final Throwable cause = ce.getCause() != null ? ce.getCause() : ce; + if (cause instanceof RuntimeException) { + throw (RuntimeException) cause; + } else if (cause instanceof Error) { + throw (Error) cause; + } else { + throw new RuntimeException(cause); + } + } + } + return value; } /** @@ -61,21 +112,24 @@ public T get(final Contextual contextual) { */ public void destroy() { // TODO We really should ensure no more instances can be added during or after this - instances.values().stream().forEach(Instance::destroy); + instances.values().forEach((final CompletableFuture future) -> { + if (future.isDone() && !future.isCompletedExceptionally()) { + future.join().destroy(); + } + }); instances.clear(); } - private class Instance { + private static class Instance { private final T instance; private final CreationalContext creationalContext; private final Contextual contextual; public Instance(final Contextual contextual, final CreationalContext creationalContext) { - this(contextual, creationalContext, contextual.create(creationalContext)); } - public Instance(Contextual contextual, CreationalContext creationalContext, T instance) { + public Instance(final Contextual contextual, final CreationalContext creationalContext, final T instance) { this.instance = instance; this.creationalContext = creationalContext; this.contextual = contextual; @@ -86,7 +140,9 @@ public T get() { } public void destroy() { - contextual.destroy(instance, creationalContext); + if (contextual != null) { + contextual.destroy(instance, creationalContext); + } } } } From fbb37552f73a25d8d00f68cab03520b7c6a89541 Mon Sep 17 00:00:00 2001 From: "Jonathan S. Fisher" Date: Sat, 16 Aug 2025 18:39:06 -0500 Subject: [PATCH 2/2] Provide Additional Fix for TODO: lock the context after it's been destroyed so it can no longer be used --- .../org/tomitribe/microscoped/core/Scope.java | 80 +++++++++++-------- 1 file changed, 48 insertions(+), 32 deletions(-) diff --git a/microscoped-core/src/main/java/org/tomitribe/microscoped/core/Scope.java b/microscoped-core/src/main/java/org/tomitribe/microscoped/core/Scope.java index 7db8dda..2a9253b 100644 --- a/microscoped-core/src/main/java/org/tomitribe/microscoped/core/Scope.java +++ b/microscoped-core/src/main/java/org/tomitribe/microscoped/core/Scope.java @@ -18,14 +18,17 @@ import java.util.Map; import java.util.concurrent.CompletableFuture; -import java.util.concurrent.CompletionException; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ExecutionException; +import javax.enterprise.context.ContextNotActiveException; import javax.enterprise.context.spi.Contextual; import javax.enterprise.context.spi.CreationalContext; +import javax.enterprise.inject.spi.Bean; class Scope { private final Map, CompletableFuture> instances = new ConcurrentHashMap<>(); + private volatile boolean closed = false; private final Key key; public Scope(final Key key) { @@ -43,30 +46,37 @@ public Key getKey() { * @return existing or newly created bean instance, never null */ public T get(final Contextual contextual, final CreationalContext creationalContext) { - CompletableFuture future = instances.get(contextual); - if (future == null) { - final CompletableFuture newFuture = new CompletableFuture<>(); - // Atomically place the new future in the map. - future = instances.putIfAbsent(contextual, newFuture); + if (closed) { + throw new ContextNotActiveException(String.format("Context not active [key=%s, contextual=%s, creationalContext=%s]", + String.valueOf(key), + (contextual instanceof Bean) ? ((Bean) contextual).getBeanClass().getName() : String.valueOf(contextual), + String.valueOf(creationalContext))); + } else { + CompletableFuture future = instances.get(contextual); if (future == null) { - // We won the race. Our future is now in the map. We are responsible for creating the bean. - future = newFuture; - try { - // The creation logic is now safely executed by only one thread. - final Instance createdInstance = new Instance<>(contextual, creationalContext); - future.complete(createdInstance); // Publish the result for other threads. - } catch (final Throwable e) { - // If creation fails, complete the future exceptionally and remove it from the map - // so that subsequent requests can try again. - future.completeExceptionally(e); - instances.remove(contextual, future); - // Re-throw the original exception. - throw e; + final CompletableFuture newFuture = new CompletableFuture<>(); + // Atomically place the new future in the map. + future = instances.putIfAbsent(contextual, newFuture); + if (future == null) { + // We won the race. Our future is now in the map. We are responsible for creating the bean. + future = newFuture; + try { + // The creation logic is now safely executed by only one thread. + final Instance createdInstance = new Instance<>(contextual, creationalContext); + future.complete(createdInstance); // Publish the result for other threads. + } catch (final Throwable e) { + // If creation fails, complete the future exceptionally and remove it from the map + // so that subsequent requests can try again. + future.completeExceptionally(e); + instances.remove(contextual, future); + // Re-throw the original exception. + throw e; + } } } + // All threads (the winner and the waiters) wait here for the result. + return fastWait(future); } - // All threads (the winner and the waiters) wait here for the result. - return fastWaitForValue(future); } /** @@ -76,11 +86,16 @@ public T get(final Contextual contextual, final CreationalContext crea * @return existing the bean instance or null */ public T get(final Contextual contextual) { - final CompletableFuture future = instances.get(contextual); - return fastWaitForValue(future); + if (closed) { + throw new ContextNotActiveException(String.format("Context not active [key=%s, contextual=%s]", String.valueOf(key), + (contextual instanceof Bean) ? ((Bean) contextual).getBeanClass().getName() : String.valueOf(contextual))); + } else { + final CompletableFuture future = instances.get(contextual); + return fastWait(future); + } } - private static T fastWaitForValue(final CompletableFuture future) throws Error { + private static T fastWait(final CompletableFuture future) throws Error { final T value; if (future == null) { value = null; @@ -90,15 +105,15 @@ private static T fastWaitForValue(final CompletableFuture future) value = instance.get(); } else { try { - final Instance instance = future.join(); + final Instance instance = future.get(); value = instance.get(); - } catch (final CompletionException ce) { - // Try to hide internal semantics here in case someone is expecting a CreationException or something - final Throwable cause = ce.getCause() != null ? ce.getCause() : ce; + } catch (final InterruptedException e) { + Thread.currentThread().interrupt(); + throw new RuntimeException(e); + } catch (final ExecutionException ee) { + final Throwable cause = ee.getCause() != null ? ee.getCause() : ee; if (cause instanceof RuntimeException) { throw (RuntimeException) cause; - } else if (cause instanceof Error) { - throw (Error) cause; } else { throw new RuntimeException(cause); } @@ -111,9 +126,10 @@ private static T fastWaitForValue(final CompletableFuture future) * Destroy all the instances in this scope */ public void destroy() { - // TODO We really should ensure no more instances can be added during or after this + closed = true; instances.values().forEach((final CompletableFuture future) -> { - if (future.isDone() && !future.isCompletedExceptionally()) { + // There's a small chance + if (!future.isCompletedExceptionally()) { future.join().destroy(); } });