ArC: fix and optimize the ContextInstances abstraction

- fixes #37958 and #38040 - use a separate lock for each bean in the generated ContextInstances - replace ContextInstances#forEach() and ContextInstances#clear() with ContextInstances#removeEach() - optimize the generated ContextInstances to significantly reduce the size of the generated bytecode
quarkusio · Jan 5, 2024 · c65bc86 · c65bc86
1 parent 5ae4d99
commit c65bc86
Show file tree

Hide file tree

Showing 9 changed files with 347 additions and 160 deletions.
diff --git a/...jects/arc/processor/src/main/java/io/quarkus/arc/processor/ContextInstancesGenerator.java b/...jects/arc/processor/src/main/java/io/quarkus/arc/processor/ContextInstancesGenerator.java
diff --git a/...endent-projects/arc/runtime/src/main/java/io/quarkus/arc/impl/AbstractInstanceHandle.java b/...endent-projects/arc/runtime/src/main/java/io/quarkus/arc/impl/AbstractInstanceHandle.java
@@ -7,17 +7,13 @@
 import jakarta.enterprise.context.Dependent;
 import jakarta.enterprise.context.spi.CreationalContext;
 
-import org.jboss.logging.Logger;
-
 import io.quarkus.arc.Arc;
 import io.quarkus.arc.InjectableBean;
 import io.quarkus.arc.InjectableContext;
 import io.quarkus.arc.InstanceHandle;
 
 abstract class AbstractInstanceHandle<T> implements InstanceHandle<T> {
 
-    private static final Logger LOGGER = Logger.getLogger(AbstractInstanceHandle.class.getName());
-
     @SuppressWarnings("rawtypes")
     private static final AtomicIntegerFieldUpdater<AbstractInstanceHandle> DESTROYED_UPDATER = AtomicIntegerFieldUpdater
             .newUpdater(AbstractInstanceHandle.class, "destroyed");

diff --git a/...pendent-projects/arc/runtime/src/main/java/io/quarkus/arc/impl/AbstractSharedContext.java b/...pendent-projects/arc/runtime/src/main/java/io/quarkus/arc/impl/AbstractSharedContext.java
@@ -84,23 +84,24 @@ public void destroy(Contextual<?> contextual) {
 
     @Override
     public synchronized void destroy() {
-        List<ContextInstanceHandle<?>> values = new LinkedList<>();
-        instances.forEach(values::add);
+        // Note that shared contexts are usually only destroyed when the app stops
+        // I.e. we don't need to use the optimized ContextInstances methods here
+        Set<ContextInstanceHandle<?>> values = instances.getAllPresent();
         if (values.isEmpty()) {
             return;
         }
         // Destroy the producers first
-        for (Iterator<ContextInstanceHandle<?>> iterator = values.iterator(); iterator.hasNext();) {
-            ContextInstanceHandle<?> instanceHandle = iterator.next();
+        for (Iterator<ContextInstanceHandle<?>> it = values.iterator(); it.hasNext();) {
+            ContextInstanceHandle<?> instanceHandle = it.next();
             if (instanceHandle.getBean().getDeclaringBean() != null) {
                 instanceHandle.destroy();
-                iterator.remove();
+                it.remove();
             }
         }
         for (ContextInstanceHandle<?> instanceHandle : values) {
             instanceHandle.destroy();
         }
-        instances.clear();
+        instances.removeEach(null);
     }
 
     @Override

diff --git a/...rojects/arc/runtime/src/main/java/io/quarkus/arc/impl/ComputingCacheContextInstances.java b/...rojects/arc/runtime/src/main/java/io/quarkus/arc/impl/ComputingCacheContextInstances.java
@@ -35,13 +35,11 @@ public Set<ContextInstanceHandle<?>> getAllPresent() {
     }
 
     @Override
-    public void clear() {
+    public void removeEach(Consumer<? super ContextInstanceHandle<?>> action) {
+        if (action != null) {
+            instances.getPresentValues().forEach(action);
+        }
         instances.clear();
     }
 
-    @Override
-    public void forEach(Consumer<? super ContextInstanceHandle<?>> handleConsumer) {
-        instances.getPresentValues().forEach(handleConsumer);
-    }
-
 }
diff --git a/...ent-projects/arc/runtime/src/main/java/io/quarkus/arc/impl/ContextInstanceHandleImpl.java b/...ent-projects/arc/runtime/src/main/java/io/quarkus/arc/impl/ContextInstanceHandleImpl.java
@@ -2,6 +2,8 @@
 
 import jakarta.enterprise.context.spi.CreationalContext;
 
+import org.jboss.logging.Logger;
+
 import io.quarkus.arc.ContextInstanceHandle;
 import io.quarkus.arc.InjectableBean;
 
@@ -12,13 +14,19 @@
  */
 public class ContextInstanceHandleImpl<T> extends EagerInstanceHandle<T> implements ContextInstanceHandle<T> {
 
+    private static final Logger LOG = Logger.getLogger(ContextInstanceHandleImpl.class);
+
     public ContextInstanceHandleImpl(InjectableBean<T> bean, T instance, CreationalContext<T> creationalContext) {
         super(bean, instance, creationalContext);
     }
 
     @Override
     public void destroy() {
-        destroyInternal();
+        try {
+            destroyInternal();
+        } catch (Exception e) {
+            LOG.error("Unable to destroy instance" + get(), e);
+        }
     }
 
 }
diff --git a/independent-projects/arc/runtime/src/main/java/io/quarkus/arc/impl/ContextInstances.java b/independent-projects/arc/runtime/src/main/java/io/quarkus/arc/impl/ContextInstances.java
@@ -8,16 +8,39 @@
 
 public interface ContextInstances {
 
+    /**
+     *
+     * @param id
+     * @param supplier
+     * @return the instance handle
+     */
     ContextInstanceHandle<?> computeIfAbsent(String id, Supplier<ContextInstanceHandle<?>> supplier);
 
+    /**
+     *
+     * @param id
+     * @return the instance handle if present, {@code null} otherwise
+     */
     ContextInstanceHandle<?> getIfPresent(String id);
 
+    /**
+     *
+     * @param id
+     * @return the removed instance handle, or {@code null}
+     */
     ContextInstanceHandle<?> remove(String id);
 
+    /**
+     *
+     * @return all instance handles
+     */
     Set<ContextInstanceHandle<?>> getAllPresent();
 
-    void clear();
-
-    void forEach(Consumer<? super ContextInstanceHandle<?>> handleConsumer);
+    /**
+     * Removes all instance handles and performs the given action (if present) for each handle.
+     *
+     * @param action
+     */
+    void removeEach(Consumer<? super ContextInstanceHandle<?>> action);
 
 }
diff --git a/independent-projects/arc/runtime/src/main/java/io/quarkus/arc/impl/RequestContext.java b/independent-projects/arc/runtime/src/main/java/io/quarkus/arc/impl/RequestContext.java
@@ -210,8 +210,7 @@ public void destroy(ContextState state) {
             if (reqState.invalidate()) {
                 // Fire an event with qualifier @BeforeDestroyed(RequestScoped.class) if there are any observers for it
                 fireIfNotEmpty(beforeDestroyedNotifier);
-                reqState.contextInstances.forEach(this::destroyContextElement);
-                reqState.contextInstances.clear();
+                reqState.contextInstances.removeEach(ContextInstanceHandle::destroy);
                 // Fire an event with qualifier @Destroyed(RequestScoped.class) if there are any observers for it
                 fireIfNotEmpty(destroyedNotifier);
             }
@@ -229,14 +228,6 @@ private static void traceDestroy(ContextState state) {
         LOG.tracef("Destroy %s%s\n\t...", state != null ? Integer.toHexString(state.hashCode()) : "", stack);
     }
 
-    private void destroyContextElement(ContextInstanceHandle<?> contextInstanceHandle) {
-        try {
-            contextInstanceHandle.destroy();
-        } catch (Exception e) {
-            throw new IllegalStateException("Unable to destroy instance" + contextInstanceHandle.get(), e);
-        }
-    }
-
     private void fireIfNotEmpty(Notifier<Object> notifier) {
         if (notifier != null && !notifier.isEmpty()) {
             try {

diff --git a/...a/io/quarkus/arc/test/contexts/application/optimized/ApplicationContextInstancesTest.java b/...a/io/quarkus/arc/test/contexts/application/optimized/ApplicationContextInstancesTest.java
@@ -4,9 +4,17 @@
 import static org.junit.jupiter.api.Assertions.assertNotEquals;
 
 import java.util.UUID;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
 
 import jakarta.annotation.PostConstruct;
+import jakarta.annotation.PreDestroy;
 import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.inject.Inject;
 
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
@@ -21,7 +29,7 @@ public class ApplicationContextInstancesTest {
 
     @RegisterExtension
     ArcTestContainer container = ArcTestContainer.builder()
-            .beanClasses(Boom.class)
+            .beanClasses(Boom.class, Bim.class)
             .optimizeContexts(true)
             .build();
 
@@ -30,15 +38,23 @@ public void testContext() {
         ArcContainer container = Arc.container();
         InstanceHandle<Boom> handle = container.instance(Boom.class);
         Boom boom = handle.get();
+        // ContextInstances#computeIfAbsent()
         String id1 = boom.ping();
         assertEquals(id1, boom.ping());
 
+        // ContextInstances#remove()
         handle.destroy();
+        // Bim bean is not destroyed
+        // ContextInstances#getAllPresent()
+        assertEquals(1, container.getActiveContext(ApplicationScoped.class).getState().getContextualInstances().size());
+
+        // Init a new instance of Boom
         String id2 = boom.ping();
         assertNotEquals(id1, id2);
         assertEquals(id2, boom.ping());
 
         InjectableContext appContext = container.getActiveContext(ApplicationScoped.class);
+        // ContextInstances#removeEach()
         appContext.destroy();
         assertNotEquals(id2, boom.ping());
     }
@@ -48,13 +64,41 @@ public static class Boom {
 
         private String id;
 
+        @Inject
+        Bim bim;
+
         String ping() {
             return id;
         }
 
         @PostConstruct
         void init() {
             id = UUID.randomUUID().toString();
+
+            ExecutorService executorService = Executors.newSingleThreadExecutor();
+            Future<?> f = executorService.submit(() -> {
+                // Force the init of the bean on a different thread
+                bim.bam();
+            });
+            try {
+                f.get(2, TimeUnit.SECONDS);
+            } catch (InterruptedException | ExecutionException | TimeoutException e) {
+                throw new IllegalStateException(e);
+            }
+            executorService.shutdownNow();
+        }
+
+        @PreDestroy
+        void destroy() {
+            throw new IllegalStateException("Boom");
+        }
+
+    }
+
+    @ApplicationScoped
+    public static class Bim {
+
+        public void bam() {
         }
 
     }

diff --git a/...test/java/io/quarkus/arc/test/contexts/request/optimized/RequestContextInstancesTest.java b/...test/java/io/quarkus/arc/test/contexts/request/optimized/RequestContextInstancesTest.java
@@ -0,0 +1,109 @@
+package io.quarkus.arc.test.contexts.request.optimized;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+
+import java.util.UUID;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+import jakarta.annotation.PostConstruct;
+import jakarta.annotation.PreDestroy;
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.enterprise.context.RequestScoped;
+import jakarta.inject.Inject;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+
+import io.quarkus.arc.Arc;
+import io.quarkus.arc.ArcContainer;
+import io.quarkus.arc.InjectableContext;
+import io.quarkus.arc.InstanceHandle;
+import io.quarkus.arc.test.ArcTestContainer;
+
+public class RequestContextInstancesTest {
+
+    @RegisterExtension
+    ArcTestContainer container = ArcTestContainer.builder()
+            .beanClasses(Boom.class, Bim.class)
+            .optimizeContexts(true)
+            .build();
+
+    @Test
+    public void testContext() {
+        ArcContainer container = Arc.container();
+        container.requestContext().activate();
+
+        InstanceHandle<Boom> handle = container.instance(Boom.class);
+        Boom boom = handle.get();
+        // ContextInstances#computeIfAbsent()
+        String id1 = boom.ping();
+        assertEquals(id1, boom.ping());
+
+        // ContextInstances#remove()
+        handle.destroy();
+        // ContextInstances#getAllPresent()
+        assertEquals(0, container.getActiveContext(RequestScoped.class).getState().getContextualInstances().size());
+
+        // Init a new instance of Boom
+        String id2 = boom.ping();
+        assertNotEquals(id1, id2);
+        assertEquals(id2, boom.ping());
+
+        InjectableContext appContext = container.getActiveContext(RequestScoped.class);
+        // ContextInstances#removeEach()
+        appContext.destroy();
+        assertNotEquals(id2, boom.ping());
+
+        container.requestContext().terminate();
+    }
+
+    @RequestScoped
+    public static class Boom {
+
+        private String id;
+
+        @Inject
+        Bim bim;
+
+        String ping() {
+            return id;
+        }
+
+        @PostConstruct
+        void init() {
+            id = UUID.randomUUID().toString();
+
+            ExecutorService executorService = Executors.newSingleThreadExecutor();
+            Future<?> f = executorService.submit(() -> {
+                // Force the init of the bean on a different thread
+                bim.bam();
+            });
+            try {
+                f.get(2, TimeUnit.SECONDS);
+            } catch (InterruptedException | ExecutionException | TimeoutException e) {
+                throw new IllegalStateException(e);
+            }
+            executorService.shutdownNow();
+        }
+
+        @PreDestroy
+        void destroy() {
+            throw new IllegalStateException("Boom");
+        }
+
+    }
+
+    @ApplicationScoped
+    public static class Bim {
+
+        public void bam() {
+        }
+
+    }
+}