This is an automated email from the ASF dual-hosted git repository. quantranhong1999 pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/james-project.git
commit 5a394e1a996bfaf43ceb5a202bcec04f18da82f8 Author: Quan Tran <[email protected]> AuthorDate: Sat Oct 3 10:15:19 2026 +0700 [FIX] Lifecycle: run @PreDestroy of provider-built singletons @PreDestroy methods were only registered for objects Guice constructs or injects itself. Singletons returned by providers (@Provides methods, provider instances or classes) were never stopped: RabbitMQ event buses, channel pools, the Cassandra session... They are now stopped by GuiceJamesServer.stop(), so a production shutdown releases them, and restarting servers in one JVM no longer leaks ~160 threads per restart, which lets tests reuse surefire forks. The SmtpGuiceProbe and ImapGuiceProbe workarounds destroying their server factories by hand are removed. Co-Authored-By: Claude Opus 5.5 <[email protected]> --- .../lifecycle/AbstractMethodTypeListener.java | 54 ++-- .../onami/lifecycle/LifeCycleStageModule.java | 77 ++++- .../ProviderBuiltSingletonStagingTest.java | 338 +++++++++++++++++++++ .../james/modules/protocols/ImapGuiceProbe.java | 6 - .../james/modules/protocols/SmtpGuiceProbe.java | 7 - 5 files changed, 439 insertions(+), 43 deletions(-) diff --git a/server/container/guice/onami/src/main/java/org/apache/james/onami/lifecycle/AbstractMethodTypeListener.java b/server/container/guice/onami/src/main/java/org/apache/james/onami/lifecycle/AbstractMethodTypeListener.java index aae0bbbc04..2b49380777 100644 --- a/server/container/guice/onami/src/main/java/org/apache/james/onami/lifecycle/AbstractMethodTypeListener.java +++ b/server/container/guice/onami/src/main/java/org/apache/james/onami/lifecycle/AbstractMethodTypeListener.java @@ -21,6 +21,7 @@ package org.apache.james.onami.lifecycle; import java.lang.annotation.Annotation; import java.lang.reflect.Method; +import java.util.ArrayList; import java.util.List; import com.google.inject.TypeLiteral; @@ -37,6 +38,25 @@ abstract class AbstractMethodTypeListener implements TypeListener { */ private static final String JAVA_PACKAGE = "java"; + /** + * Lists the input klass and its superclasses, stopping at the first {@code java} package class. + * + * @param klass the class to start from. + * @return the classes to scan for lifecycle methods, from the input klass up. + */ + static List<Class<?>> lifecycleHierarchy(Class<?> klass) { + List<Class<?>> hierarchy = new ArrayList<>(); + for (Class<?> current = klass; current != null && !isJavaClass(current); current = current.getSuperclass()) { + hierarchy.add(current); + } + return hierarchy; + } + + private static boolean isJavaClass(Class<?> klass) { + Package pkg = klass.getPackage(); + return pkg != null && pkg.getName().startsWith(JAVA_PACKAGE); + } + /** * The lifecycle annotations to search on methods in the order to be searched. */ @@ -53,36 +73,20 @@ abstract class AbstractMethodTypeListener implements TypeListener { @Override public final <I> void hear(TypeLiteral<I> type, TypeEncounter<I> encounter) { - hear(type, type.getRawType(), encounter); - } - - /** - * Allows traverse the input klass hierarchy. - * - * @param parentType the owning type being heard - * @param klass encountered by Guice. - * @param encounter the injection context. - */ - private <I> void hear(final TypeLiteral<I> parentType, Class<? super I> klass, TypeEncounter<I> encounter) { - Package pkg; - if (klass == null || ((pkg = klass.getPackage()) != null && pkg.getName().startsWith(JAVA_PACKAGE))) { - return; - } + for (Class<?> klass : lifecycleHierarchy(type.getRawType())) { + for (Class<? extends Annotation> annotationType : annotationTypes) { + for (Method method : klass.getDeclaredMethods()) { + if (method.isAnnotationPresent(annotationType)) { + if (method.getParameterTypes().length != 0) { + encounter.addError("Annotated methods with @%s must not accept any argument, found %s", + annotationType.getName(), method); + } - for (Class<? extends Annotation> annotationType : annotationTypes) { - for (Method method : klass.getDeclaredMethods()) { - if (method.isAnnotationPresent(annotationType)) { - if (method.getParameterTypes().length != 0) { - encounter.addError("Annotated methods with @%s must not accept any argument, found %s", - annotationType.getName(), method); + hear(method, type, encounter, annotationType); } - - hear(method, parentType, encounter, annotationType); } } } - - hear(parentType, klass.getSuperclass(), encounter); } /** diff --git a/server/container/guice/onami/src/main/java/org/apache/james/onami/lifecycle/LifeCycleStageModule.java b/server/container/guice/onami/src/main/java/org/apache/james/onami/lifecycle/LifeCycleStageModule.java index a5b3d5f799..2f13a3f19a 100644 --- a/server/container/guice/onami/src/main/java/org/apache/james/onami/lifecycle/LifeCycleStageModule.java +++ b/server/container/guice/onami/src/main/java/org/apache/james/onami/lifecycle/LifeCycleStageModule.java @@ -24,14 +24,26 @@ import static java.util.Arrays.asList; import java.lang.annotation.Annotation; import java.lang.reflect.Method; +import java.lang.reflect.Modifier; import java.lang.reflect.ParameterizedType; import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashSet; +import java.util.IdentityHashMap; import java.util.List; +import java.util.Map; +import java.util.Set; +import com.google.inject.Binding; import com.google.inject.Key; +import com.google.inject.Scopes; import com.google.inject.TypeLiteral; +import com.google.inject.matcher.AbstractMatcher; import com.google.inject.matcher.Matcher; import com.google.inject.spi.InjectionListener; +import com.google.inject.spi.ProviderInstanceBinding; +import com.google.inject.spi.ProviderKeyBinding; +import com.google.inject.spi.ProvisionListener; import com.google.inject.spi.TypeEncounter; import com.google.inject.util.Types; @@ -68,6 +80,41 @@ public abstract class LifeCycleStageModule extends LifeCycleModule { return stagerType; } + private static boolean isProviderBuiltSingleton(Binding<?> binding) { + return (binding instanceof ProviderInstanceBinding || binding instanceof ProviderKeyBinding) && Scopes.isSingleton(binding); + } + + private static List<Method> stageMethods(Class<?> klass, Class<? extends Annotation> stage) { + return AbstractMethodTypeListener.lifecycleHierarchy(klass).stream() + .flatMap(type -> Arrays.stream(type.getDeclaredMethods())) + .filter(method -> method.isAnnotationPresent(stage) && method.getParameterCount() == 0) + .toList(); + } + + // Invoking an overridden declaration runs the override: both must be registered only once + private static Object overrideKey(Method method) { + if (Modifier.isPrivate(method.getModifiers())) { + return method; + } + return List.of(method.getName(), List.of(method.getParameterTypes())); + } + + private static boolean isFirstRegistration(Map<Object, Set<Object>> registeredMethods, Object instance, Method stageMethod) { + synchronized (registeredMethods) { + return registeredMethods.computeIfAbsent(instance, any -> new HashSet<>()) + .add(overrideKey(stageMethod)); + } + } + + private static void registerOnce(Map<Object, Set<Object>> registeredMethods, Stager<?> stager, StageableTypeMapper typeMapper, + Object instance, Method stageMethod, TypeLiteral<?> type) { + if (isFirstRegistration(registeredMethods, instance, stageMethod)) { + Stageable stageable = new StageableMethod(stageMethod, instance); + stager.register(stageable); + typeMapper.registerType(stageable, type); + } + } + @Override protected final void configure() { if (bindings != null) { @@ -87,17 +134,37 @@ public abstract class LifeCycleStageModule extends LifeCycleModule { private <A extends Annotation> void bind(BindingBuilder<A> binding) { final Stager<A> stager = binding.stager; final StageableTypeMapper typeMapper = binding.typeMapper; + // Shared by both listeners: an instance both injected and returned by a provider, or exposed under several keys, is staged once + final Map<Object, Set<Object>> registeredMethods = new IdentityHashMap<>(); bind(type(stager.getStage())).toInstance(stager); bindListener(binding.typeMatcher, new AbstractMethodTypeListener(asList(stager.getStage())) { @Override protected <I> void hear(final Method stageMethod, final TypeLiteral<I> parentType, final TypeEncounter<I> encounter, final Class<? extends Annotation> annotationType) { - encounter.register((InjectionListener<I>) injectee -> { - Stageable stageable = new StageableMethod(stageMethod, injectee); - stager.register(stageable); - typeMapper.registerType(stageable, parentType); - }); + encounter.register((InjectionListener<I>) injectee -> + registerOnce(registeredMethods, stager, typeMapper, injectee, stageMethod, parentType)); + } + }); + + // Guice does not inject objects returned by providers (provider methods, instances or classes), so the type listener above never hears them + bindListener(new AbstractMatcher<Binding<?>>() { + @Override + public boolean matches(Binding<?> candidate) { + return isProviderBuiltSingleton(candidate); + } + }, new ProvisionListener() { + @Override + public <T> void onProvision(ProvisionInvocation<T> provision) { + T instance = provision.provision(); + if (instance == null) { + return; + } + TypeLiteral<?> instanceType = TypeLiteral.get(instance.getClass()); + if (binding.typeMatcher.matches(instanceType)) { + stageMethods(instance.getClass(), stager.getStage()) + .forEach(stageMethod -> registerOnce(registeredMethods, stager, typeMapper, instance, stageMethod, instanceType)); + } } }); } diff --git a/server/container/guice/onami/src/test/java/org/apache/james/onami/lifecycle/ProviderBuiltSingletonStagingTest.java b/server/container/guice/onami/src/test/java/org/apache/james/onami/lifecycle/ProviderBuiltSingletonStagingTest.java new file mode 100644 index 0000000000..a28c82e32a --- /dev/null +++ b/server/container/guice/onami/src/test/java/org/apache/james/onami/lifecycle/ProviderBuiltSingletonStagingTest.java @@ -0,0 +1,338 @@ +/**************************************************************** + * Licensed to the Apache Software Foundation (ASF) under one * + * or more contributor license agreements. See the NOTICE file * + * distributed with this work for additional information * + * regarding copyright ownership. The ASF licenses this file * + * to you under the Apache License, Version 2.0 (the * + * "License"); you may not use this file except in compliance * + * with the License. You may obtain a copy of the License at * + * * + * http://www.apache.org/licenses/LICENSE-2.0 * + * * + * Unless required by applicable law or agreed to in writing, * + * software distributed under the License is distributed on an * + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY * + * KIND, either express or implied. See the License for the * + * specific language governing permissions and limitations * + * under the License. * + ****************************************************************/ + +package org.apache.james.onami.lifecycle; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.ArrayList; +import java.util.List; + +import jakarta.annotation.PreDestroy; +import jakarta.inject.Inject; +import jakarta.inject.Provider; +import jakarta.inject.Singleton; + +import org.junit.jupiter.api.Test; + +import com.google.inject.AbstractModule; +import com.google.inject.Guice; +import com.google.inject.Injector; +import com.google.inject.Module; +import com.google.inject.Provides; +import com.google.inject.Scopes; +import com.google.inject.TypeLiteral; + +class ProviderBuiltSingletonStagingTest { + interface Stoppable { + } + + static class StopRecorder implements Stoppable { + private final String name; + private final List<String> stops; + + StopRecorder(String name, List<String> stops) { + this.name = name; + this.stops = stops; + } + + @PreDestroy + void stop() { + stops.add(name); + } + } + + static class InheritingStopRecorder extends StopRecorder { + InheritingStopRecorder(String name, List<String> stops) { + super(name, stops); + } + } + + static class InjectedStopRecorder extends StopRecorder { + @Inject + InjectedStopRecorder(List<String> stops) { + super("injected", stops); + } + } + + static class OverridingStopRecorder extends StopRecorder { + OverridingStopRecorder(String name, List<String> stops) { + super(name, stops); + } + + @Override + @PreDestroy + void stop() { + super.stop(); + } + } + + static class InjectedOverridingStopRecorder extends StopRecorder { + @Inject + InjectedOverridingStopRecorder(List<String> stops) { + super("injected-overriding", stops); + } + + @Override + @PreDestroy + void stop() { + super.stop(); + } + } + + static class PrivateStopBase { + final List<String> stops; + + PrivateStopBase(List<String> stops) { + this.stops = stops; + } + + @PreDestroy + private void stop() { + stops.add("base"); + } + } + + static class PrivateStopDerived extends PrivateStopBase { + PrivateStopDerived(List<String> stops) { + super(stops); + } + + @PreDestroy + private void stop() { + stops.add("derived"); + } + } + + static class StopRecorderProvider implements Provider<Stoppable> { + private final List<String> stops; + + @Inject + StopRecorderProvider(List<String> stops) { + this.stops = stops; + } + + @Override + public Stoppable get() { + return new StopRecorder("provider-class", stops); + } + } + + private final List<String> stops = new ArrayList<>(); + private final PreDestroyModule preDestroyModule = new PreDestroyModule(); + + private Injector injector(Module module) { + return Guice.createInjector(preDestroyModule, binder -> binder.bind(new TypeLiteral<List<String>>() { }).toInstance(stops), module); + } + + @Test + void preDestroyOfProviderBuiltSingletonShouldBeCalledOnce() { + Injector injector = injector(new AbstractModule() { + @Provides + @Singleton + StopRecorder stopRecorder() { + return new StopRecorder("provided", stops); + } + }); + injector.getInstance(StopRecorder.class); + injector.getInstance(StopRecorder.class); + + preDestroyModule.getStager().stage(); + + assertThat(stops).containsExactly("provided"); + } + + @Test + void inheritedPreDestroyOfProviderBuiltSingletonShouldBeCalled() { + Injector injector = injector(new AbstractModule() { + @Provides + @Singleton + StopRecorder stopRecorder() { + return new InheritingStopRecorder("inheriting", stops); + } + }); + injector.getInstance(StopRecorder.class); + + preDestroyModule.getStager().stage(); + + assertThat(stops).containsExactly("inheriting"); + } + + @Test + void preDestroyOfConstructorInjectedSingletonShouldBeCalledOnce() { + Injector injector = injector(binder -> binder.bind(InjectedStopRecorder.class).in(Scopes.SINGLETON)); + injector.getInstance(InjectedStopRecorder.class); + injector.getInstance(InjectedStopRecorder.class); + + preDestroyModule.getStager().stage(); + + assertThat(stops).containsExactly("injected"); + } + + @Test + void preDestroyOfInjectedSingletonReturnedByProviderShouldBeCalledOnce() { + Injector injector = injector(new AbstractModule() { + @Override + protected void configure() { + bind(InjectedStopRecorder.class).in(Scopes.SINGLETON); + } + + @Provides + @Singleton + Stoppable stoppable(InjectedStopRecorder stopRecorder) { + return stopRecorder; + } + }); + injector.getInstance(Stoppable.class); + injector.getInstance(InjectedStopRecorder.class); + + preDestroyModule.getStager().stage(); + + assertThat(stops).containsExactly("injected"); + } + + @Test + void preDestroyOfProviderBuiltSingletonExposedUnderTwoKeysShouldBeCalledOnce() { + Injector injector = injector(new AbstractModule() { + @Provides + @Singleton + StopRecorder stopRecorder() { + return new StopRecorder("provided", stops); + } + + @Provides + @Singleton + Stoppable stoppable(StopRecorder stopRecorder) { + return stopRecorder; + } + }); + injector.getInstance(Stoppable.class); + injector.getInstance(StopRecorder.class); + + preDestroyModule.getStager().stage(); + + assertThat(stops).containsExactly("provided"); + } + + @Test + void providerBuiltSingletonShouldBeStoppedBeforeItsDependencies() { + Injector injector = injector(new AbstractModule() { + @Provides + @Singleton + StopRecorder dependency() { + return new StopRecorder("dependency", stops); + } + + @Provides + @Singleton + Stoppable dependent(StopRecorder dependency) { + return new StopRecorder("dependent", stops); + } + }); + injector.getInstance(Stoppable.class); + + preDestroyModule.getStager().stage(); + + assertThat(stops).containsExactly("dependent", "dependency"); + } + + @Test + void preDestroyOfUnscopedProviderBuiltObjectShouldNotBeCalled() { + Injector injector = injector(new AbstractModule() { + @Provides + StopRecorder stopRecorder() { + return new StopRecorder("unscoped", stops); + } + }); + injector.getInstance(StopRecorder.class); + + preDestroyModule.getStager().stage(); + + assertThat(stops).isEmpty(); + } + + @Test + void preDestroyOfProviderBuiltSingletonInjectedLaterShouldBeCalledOnce() { + Injector injector = injector(new AbstractModule() { + @Provides + @Singleton + StopRecorder stopRecorder() { + return new StopRecorder("provided", stops); + } + }); + injector.injectMembers(injector.getInstance(StopRecorder.class)); + + preDestroyModule.getStager().stage(); + + assertThat(stops).containsExactly("provided"); + } + + @Test + void overriddenPreDestroyOfProviderBuiltSingletonShouldBeCalledOnce() { + Injector injector = injector(new AbstractModule() { + @Provides + @Singleton + StopRecorder stopRecorder() { + return new OverridingStopRecorder("overriding", stops); + } + }); + injector.getInstance(StopRecorder.class); + + preDestroyModule.getStager().stage(); + + assertThat(stops).containsExactly("overriding"); + } + + @Test + void overriddenPreDestroyOfConstructorInjectedSingletonShouldBeCalledOnce() { + Injector injector = injector(binder -> binder.bind(InjectedOverridingStopRecorder.class).in(Scopes.SINGLETON)); + injector.getInstance(InjectedOverridingStopRecorder.class); + + preDestroyModule.getStager().stage(); + + assertThat(stops).containsExactly("injected-overriding"); + } + + @Test + void privatePreDestroyMethodsOfSubclassAndSuperclassShouldBothBeCalled() { + Injector injector = injector(new AbstractModule() { + @Provides + @Singleton + PrivateStopBase privateStop() { + return new PrivateStopDerived(stops); + } + }); + injector.getInstance(PrivateStopBase.class); + + preDestroyModule.getStager().stage(); + + assertThat(stops).containsExactlyInAnyOrder("derived", "base"); + } + + @Test + void preDestroyOfSingletonBuiltByProviderClassShouldBeCalledOnce() { + Injector injector = injector(binder -> binder.bind(Stoppable.class).toProvider(StopRecorderProvider.class).in(Scopes.SINGLETON)); + injector.getInstance(Stoppable.class); + injector.getInstance(Stoppable.class); + + preDestroyModule.getStager().stage(); + + assertThat(stops).containsExactly("provider-class"); + } +} diff --git a/server/container/guice/protocols/imap/src/main/java/org/apache/james/modules/protocols/ImapGuiceProbe.java b/server/container/guice/protocols/imap/src/main/java/org/apache/james/modules/protocols/ImapGuiceProbe.java index a216fbfae4..1c2165c2df 100644 --- a/server/container/guice/protocols/imap/src/main/java/org/apache/james/modules/protocols/ImapGuiceProbe.java +++ b/server/container/guice/protocols/imap/src/main/java/org/apache/james/modules/protocols/ImapGuiceProbe.java @@ -22,7 +22,6 @@ import java.net.InetSocketAddress; import java.util.Optional; import java.util.function.Predicate; -import jakarta.annotation.PreDestroy; import jakarta.inject.Inject; import org.apache.james.imapserver.netty.IMAPServerFactory; @@ -39,11 +38,6 @@ public class ImapGuiceProbe implements GuiceProbe { this.imapServerFactory = imapServerFactory; } - @PreDestroy - void destroy() { - imapServerFactory.destroy(); - } - public int getImapPort() { return getPort(Predicate.not(AbstractConfigurableAsyncServer::getStartTLSSupported)) .orElseThrow(() -> new IllegalStateException("IMAP server not defined")); diff --git a/server/container/guice/protocols/smtp/src/main/java/org/apache/james/modules/protocols/SmtpGuiceProbe.java b/server/container/guice/protocols/smtp/src/main/java/org/apache/james/modules/protocols/SmtpGuiceProbe.java index e96153afbc..9c67ced130 100644 --- a/server/container/guice/protocols/smtp/src/main/java/org/apache/james/modules/protocols/SmtpGuiceProbe.java +++ b/server/container/guice/protocols/smtp/src/main/java/org/apache/james/modules/protocols/SmtpGuiceProbe.java @@ -24,7 +24,6 @@ import java.net.InetSocketAddress; import java.util.function.Function; import java.util.function.Predicate; -import jakarta.annotation.PreDestroy; import jakarta.inject.Inject; import org.apache.james.protocols.lib.netty.AbstractConfigurableAsyncServer; @@ -57,12 +56,6 @@ public class SmtpGuiceProbe implements GuiceProbe { this.smtpServerFactory = smtpServerFactory; } - @PreDestroy - void destroy() { - // SMTPServerFactory is provided through a factory method; dispose it explicitly on Guice shutdown. - smtpServerFactory.destroy(); - } - public Port getSmtpPort() { return getPort(server -> true); } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
