After use, destroy Dependent-scoped instances obtained via CdiLookupS…

…ervice
smallrye · Mar 9, 2022 · c002115 · c002115
1 parent e9c3094
commit c002115
Show file tree

Hide file tree

Showing 7 changed files with 176 additions and 11 deletions.
diff --git a/server/implementation-cdi/src/main/java/io/smallrye/graphql/cdi/CDIManagedInstance.java b/server/implementation-cdi/src/main/java/io/smallrye/graphql/cdi/CDIManagedInstance.java
@@ -0,0 +1,30 @@
+package io.smallrye.graphql.cdi;
+
+import javax.enterprise.inject.Instance;
+
+import io.smallrye.graphql.spi.ManagedInstance;
+
+public class CDIManagedInstance<T> implements ManagedInstance<T> {
+
+    private final Instance<T> instance;
+    private final T object;
+    private final boolean isDependentScoped;
+
+    CDIManagedInstance(Instance<T> instance, boolean isDependentScoped) {
+        this.instance = instance;
+        this.isDependentScoped = isDependentScoped;
+        this.object = instance.get();
+    }
+
+    @Override
+    public T get() {
+        return object;
+    }
+
+    @Override
+    public void destroyIfNecessary() {
+        if (isDependentScoped) {
+            instance.destroy(object);
+        }
+    }
+}
diff --git a/server/implementation-cdi/src/main/java/io/smallrye/graphql/cdi/CdiLookupService.java b/server/implementation-cdi/src/main/java/io/smallrye/graphql/cdi/CdiLookupService.java
@@ -1,8 +1,15 @@
 package io.smallrye.graphql.cdi;
 
+import java.util.Set;
+
+import javax.enterprise.context.Dependent;
+import javax.enterprise.inject.AmbiguousResolutionException;
+import javax.enterprise.inject.UnsatisfiedResolutionException;
+import javax.enterprise.inject.spi.Bean;
 import javax.enterprise.inject.spi.CDI;
 
 import io.smallrye.graphql.spi.LookupService;
+import io.smallrye.graphql.spi.ManagedInstance;
 
 /**
  * Lookup service that gets the beans via CDI
@@ -23,8 +30,21 @@ public Class<?> getClass(Class<?> declaringClass) {
     }
 
     @Override
-    public Object getInstance(Class<?> declaringClass) {
-        return CDI.current().select(declaringClass).get();
+    public <T> ManagedInstance<T> getInstance(Class<T> declaringClass) {
+        CDI<Object> cdi = CDI.current();
+        Bean<?> bean = getExactlyOneObject(cdi.getBeanManager().getBeans(declaringClass));
+        boolean isDependentScope = bean.getScope().equals(Dependent.class);
+        return new CDIManagedInstance<>(cdi.select(declaringClass), isDependentScope);
+    }
+
+    private <T> T getExactlyOneObject(Set<T> set) {
+        if (set.size() > 1) {
+            throw new AmbiguousResolutionException();
+        }
+        if (set.size() == 0) {
+            throw new UnsatisfiedResolutionException();
+        }
+        return set.iterator().next();
     }
 
     @Override

diff --git a/...mplementation-cdi/src/main/java/io/smallrye/graphql/cdi/validation/ValidationService.java b/...mplementation-cdi/src/main/java/io/smallrye/graphql/cdi/validation/ValidationService.java
@@ -48,8 +48,7 @@ public String getConfigKey() {
 
     private ValidatorFactory getValidatorFactory() {
         try {
-            ValidatorFactory validatorFactory = (ValidatorFactory) lookupService.getInstance(ValidatorFactory.class);
-            return validatorFactory;
+            return lookupService.getInstance(ValidatorFactory.class).get();
         } catch (Exception t) {
             return Validation.buildDefaultValidatorFactory();
         }

diff --git a/...ion/src/main/java/io/smallrye/graphql/execution/datafetcher/helper/ReflectionInvoker.java b/...ion/src/main/java/io/smallrye/graphql/execution/datafetcher/helper/ReflectionInvoker.java
@@ -20,6 +20,9 @@
 import io.smallrye.graphql.execution.event.InvokeInfo;
 import io.smallrye.graphql.spi.ClassloadingService;
 import io.smallrye.graphql.spi.LookupService;
+import io.smallrye.graphql.spi.ManagedInstance;
+import io.smallrye.mutiny.Multi;
+import io.smallrye.mutiny.Uni;
 
 /**
  * Invoke methods using reflection
@@ -82,10 +85,22 @@ public <T> T invoke(Object... arguments) throws Exception {
             arguments = injectContext(arguments);
         }
         try {
-            Object operationInstance = lookupService.getInstance(operationClass);
-            eventEmitter.fireBeforeMethodInvoke(new InvokeInfo(operationInstance, method, arguments));
-
-            return (T) this.method.invoke(operationInstance, arguments);
+            ManagedInstance<?> operationInstance = lookupService.getInstance(operationClass);
+            Object operationInstance1 = operationInstance.get();
+            eventEmitter.fireBeforeMethodInvoke(new InvokeInfo(operationInstance1, method, arguments));
+            T result = (T) method.invoke(operationInstance1, arguments);
+            if (result instanceof Uni) {
+                return (T) ((Uni) result).onTermination().invoke(() -> {
+                    operationInstance.destroyIfNecessary();
+                });
+            } else if (result instanceof Multi) {
+                return (T) ((Multi) result).onTermination().invoke(() -> {
+                    operationInstance.destroyIfNecessary();
+                });
+            } else {
+                operationInstance.destroyIfNecessary();
+                return result;
+            }
         } catch (InvocationTargetException ex) {
             //Invoked method has thrown something, unwrap
             Throwable throwable = ex.getCause();

diff --git a/server/implementation/src/main/java/io/smallrye/graphql/spi/LookupService.java b/server/implementation/src/main/java/io/smallrye/graphql/spi/LookupService.java
@@ -38,7 +38,11 @@ static LookupService load() {
 
     Class<?> getClass(Class<?> declaringClass);
 
-    Object getInstance(Class<?> declaringClass);
+    /**
+     * Obtain an instance of the requested class. Don't forget to call `destroyIfNecessary()` on it after use
+     * to avoid leaks!
+     */
+    <T> ManagedInstance<T> getInstance(Class<T> declaringClass);
 
     boolean isResolvable(Class<?> declaringClass);
 
@@ -59,9 +63,9 @@ public Class<?> getClass(Class<?> declaringClass) {
         }
 
         @Override
-        public Object getInstance(Class<?> declaringClass) {
+        public <T> ManagedInstance<T> getInstance(Class<T> declaringClass) {
             try {
-                return declaringClass.getConstructor().newInstance();
+                return new DefaultManagedInstance<T>(declaringClass.getConstructor().newInstance());
             } catch (NoSuchMethodException | SecurityException | InstantiationException | IllegalAccessException
                     | IllegalArgumentException | InvocationTargetException ex) {
                 throw msg.countNotGetInstance(ex);
@@ -73,4 +77,22 @@ public boolean isResolvable(Class<?> declaringClass) {
             return true;
         }
     }
+
+    class DefaultManagedInstance<T> implements ManagedInstance<T> {
+        private final T instance;
+
+        public DefaultManagedInstance(T instance) {
+            this.instance = instance;
+        }
+
+        @Override
+        public T get() {
+            return instance;
+        }
+
+        @Override
+        public void destroyIfNecessary() {
+            // nothing
+        }
+    }
 }
diff --git a/server/implementation/src/main/java/io/smallrye/graphql/spi/ManagedInstance.java b/server/implementation/src/main/java/io/smallrye/graphql/spi/ManagedInstance.java
@@ -0,0 +1,9 @@
+package io.smallrye.graphql.spi;
+
+public interface ManagedInstance<T> {
+
+    T get();
+
+    void destroyIfNecessary();
+
+}
diff --git a/...ration-tests/src/test/java/io/smallrye/graphql/tests/issue1307/DependentScopeApiTest.java b/...ration-tests/src/test/java/io/smallrye/graphql/tests/issue1307/DependentScopeApiTest.java
@@ -0,0 +1,70 @@
+package io.smallrye.graphql.tests.issue1307;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.net.URL;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import javax.annotation.PreDestroy;
+import javax.enterprise.context.Dependent;
+
+import org.eclipse.microprofile.graphql.GraphQLApi;
+import org.eclipse.microprofile.graphql.Query;
+import org.jboss.arquillian.container.test.api.Deployment;
+import org.jboss.arquillian.junit.Arquillian;
+import org.jboss.arquillian.test.api.ArquillianResource;
+import org.jboss.shrinkwrap.api.ShrinkWrap;
+import org.jboss.shrinkwrap.api.asset.EmptyAsset;
+import org.jboss.shrinkwrap.api.spec.WebArchive;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import graphql.Assert;
+import io.smallrye.graphql.tests.GraphQLAssured;
+
+/**
+ * Test to verify that a @Dependent-scoped GraphQL API will get each instance destroyed after use,
+ * to prevent leaks
+ */
+@RunWith(Arquillian.class)
+public class DependentScopeApiTest {
+
+    @Deployment
+    public static WebArchive deployment() {
+        return ShrinkWrap.create(WebArchive.class, "dependent-scope-test.war")
+                .addAsWebInfResource(EmptyAsset.INSTANCE, "beans.xml")
+                .addClasses(DependentScopedApi.class);
+    }
+
+    @ArquillianResource
+    URL testingURL;
+
+    @Test
+    public void test() {
+        GraphQLAssured graphQLAssured = new GraphQLAssured(testingURL);
+        String response = graphQLAssured
+                .post("query {hello}");
+
+        assertThat(response).isEqualTo("{\"data\":{\"hello\":\"hi\"}}");
+        Assert.assertTrue(DependentScopedApi.DESTROYED.get(),
+                () -> "Instance of the API class has to be destroyed after each request");
+    }
+
+    @GraphQLApi
+    @Dependent
+    public static class DependentScopedApi {
+
+        public static AtomicBoolean DESTROYED = new AtomicBoolean();
+
+        @Query
+        public String hello() {
+            return "hi";
+        }
+
+        @PreDestroy
+        public void onDestroy() {
+            DESTROYED.set(true);
+        }
+
+    }
+}