From 7ca64338693b29ee47a68116a93c6ce07a554a27 Mon Sep 17 00:00:00 2001 From: Maciej Walkowiak Date: Fri, 11 Feb 2022 23:47:01 +0100 Subject: [PATCH 1/3] Fix potential memory leaks. --- sentry-spring/api/sentry-spring.api | 3 ++- .../spring/SentryInitBeanPostProcessor.java | 14 ++++++++++++-- sentry/src/main/java/io/sentry/HostnameCache.java | 15 ++++++++++++++- .../main/java/io/sentry/MainEventProcessor.java | 2 +- sentry/src/main/java/io/sentry/Sentry.java | 2 ++ 5 files changed, 31 insertions(+), 5 deletions(-) diff --git a/sentry-spring/api/sentry-spring.api b/sentry-spring/api/sentry-spring.api index f8b21e455ec..82186949ac6 100644 --- a/sentry-spring/api/sentry-spring.api +++ b/sentry-spring/api/sentry-spring.api @@ -27,8 +27,9 @@ public class io/sentry/spring/SentryHubRegistrar : org/springframework/context/a public fun registerBeanDefinitions (Lorg/springframework/core/type/AnnotationMetadata;Lorg/springframework/beans/factory/support/BeanDefinitionRegistry;)V } -public class io/sentry/spring/SentryInitBeanPostProcessor : org/springframework/beans/factory/config/BeanPostProcessor, org/springframework/context/ApplicationContextAware { +public class io/sentry/spring/SentryInitBeanPostProcessor : org/springframework/beans/factory/DisposableBean, org/springframework/beans/factory/config/BeanPostProcessor, org/springframework/context/ApplicationContextAware { public fun ()V + public fun destroy ()V public fun postProcessAfterInitialization (Ljava/lang/Object;Ljava/lang/String;)Ljava/lang/Object; public fun setApplicationContext (Lorg/springframework/context/ApplicationContext;)V } diff --git a/sentry-spring/src/main/java/io/sentry/spring/SentryInitBeanPostProcessor.java b/sentry-spring/src/main/java/io/sentry/spring/SentryInitBeanPostProcessor.java index 25d92423ff7..0fc8a27f531 100644 --- a/sentry-spring/src/main/java/io/sentry/spring/SentryInitBeanPostProcessor.java +++ b/sentry-spring/src/main/java/io/sentry/spring/SentryInitBeanPostProcessor.java @@ -10,13 +10,18 @@ import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.springframework.beans.BeansException; +import org.springframework.beans.factory.DisposableBean; import org.springframework.beans.factory.config.BeanPostProcessor; import org.springframework.context.ApplicationContext; import org.springframework.context.ApplicationContextAware; -/** Initializes Sentry after all beans are registered. */ +/** + * Initializes Sentry after all beans are registered. Closes Sentry on Spring application context + * destroy. + */ @Open -public class SentryInitBeanPostProcessor implements BeanPostProcessor, ApplicationContextAware { +public class SentryInitBeanPostProcessor + implements BeanPostProcessor, ApplicationContextAware, DisposableBean { private @Nullable ApplicationContext applicationContext; @Override @@ -68,4 +73,9 @@ public void setApplicationContext(final @NotNull ApplicationContext applicationC throws BeansException { this.applicationContext = applicationContext; } + + @Override + public void destroy() { + Sentry.close(); + } } diff --git a/sentry/src/main/java/io/sentry/HostnameCache.java b/sentry/src/main/java/io/sentry/HostnameCache.java index 6d22024cb13..9de35ee1f03 100644 --- a/sentry/src/main/java/io/sentry/HostnameCache.java +++ b/sentry/src/main/java/io/sentry/HostnameCache.java @@ -21,12 +21,18 @@ * by keeping track of the hostname for a period defined during the construction. For performance * purposes, the operation of retrieving the hostname will automatically fail after a period of time * defined by {@link #GET_HOSTNAME_TIMEOUT} without result. + * + *

HostnameCache is a singleton and its instance should be obtained through {@link + * HostnameCache#getInstance()}. */ final class HostnameCache { private static final long HOSTNAME_CACHE_DURATION = TimeUnit.HOURS.toMillis(5); /** Time before the get hostname operation times out (in ms). */ private static final long GET_HOSTNAME_TIMEOUT = TimeUnit.SECONDS.toMillis(1); + + @Nullable private static HostnameCache INSTANCE; + /** Time for which the cache is kept. */ private final long cacheDuration; /** Current value for hostname (might change over time). */ @@ -41,7 +47,14 @@ final class HostnameCache { private final @NotNull ExecutorService executorService = Executors.newSingleThreadExecutor(new HostnameCacheThreadFactory()); - public HostnameCache() { + static @NotNull HostnameCache getInstance() { + if (INSTANCE == null) { + INSTANCE = new HostnameCache(); + } + return INSTANCE; + } + + private HostnameCache() { this(HOSTNAME_CACHE_DURATION); } diff --git a/sentry/src/main/java/io/sentry/MainEventProcessor.java b/sentry/src/main/java/io/sentry/MainEventProcessor.java index ebe8726a9e8..72372c6d873 100644 --- a/sentry/src/main/java/io/sentry/MainEventProcessor.java +++ b/sentry/src/main/java/io/sentry/MainEventProcessor.java @@ -34,7 +34,7 @@ public final class MainEventProcessor implements EventProcessor, Closeable { private final @Nullable HostnameCache hostnameCache; MainEventProcessor(final @NotNull SentryOptions options) { - this(options, options.isAttachServerName() ? new HostnameCache() : null); + this(options, options.isAttachServerName() ? HostnameCache.getInstance() : null); } MainEventProcessor( diff --git a/sentry/src/main/java/io/sentry/Sentry.java b/sentry/src/main/java/io/sentry/Sentry.java index e4f1cb64e75..c4fdf38dce3 100644 --- a/sentry/src/main/java/io/sentry/Sentry.java +++ b/sentry/src/main/java/io/sentry/Sentry.java @@ -237,6 +237,8 @@ private static boolean initConfigurations(final @NotNull SentryOptions options) public static synchronized void close() { final IHub hub = getCurrentHub(); mainHub = NoOpHub.getInstance(); + // remove thread local to avoid memory leak + currentHub.remove(); hub.close(); } From 5021740a81d4fa442d15b2a257a92ac55c77d786 Mon Sep 17 00:00:00 2001 From: Maciej Walkowiak Date: Fri, 11 Feb 2022 23:49:45 +0100 Subject: [PATCH 2/3] Changelog. --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a97f543c36..2dc63b1b457 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## Unreleased * Fix: Do not include stacktrace frames into Timber message (#1898) +* Fix: Potential memory leaks (#1909) Breaking changes: `Timber.tag` is no longer supported by our [Timber integration](https://docs.sentry.io/platforms/android/configuration/integrations/timber/) and will not appear on Sentry for error events. From aa3588956983071efc4bc1aa628e28c2b658581c Mon Sep 17 00:00:00 2001 From: Maciej Walkowiak Date: Mon, 14 Feb 2022 07:30:11 +0100 Subject: [PATCH 3/3] Add test. --- .../spring/SentryInitBeanPostProcessor.java | 16 +++++++++- .../spring/SentryInitBeanPostProcessorTest.kt | 30 +++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) create mode 100644 sentry-spring/src/test/kotlin/io/sentry/spring/SentryInitBeanPostProcessorTest.kt diff --git a/sentry-spring/src/main/java/io/sentry/spring/SentryInitBeanPostProcessor.java b/sentry-spring/src/main/java/io/sentry/spring/SentryInitBeanPostProcessor.java index 0fc8a27f531..361e273ab5c 100644 --- a/sentry-spring/src/main/java/io/sentry/spring/SentryInitBeanPostProcessor.java +++ b/sentry-spring/src/main/java/io/sentry/spring/SentryInitBeanPostProcessor.java @@ -2,11 +2,14 @@ import com.jakewharton.nopen.annotation.Open; import io.sentry.EventProcessor; +import io.sentry.HubAdapter; +import io.sentry.IHub; import io.sentry.ITransportFactory; import io.sentry.Integration; import io.sentry.Sentry; import io.sentry.SentryOptions; import io.sentry.SentryOptions.TracesSamplerCallback; +import io.sentry.util.Objects; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.springframework.beans.BeansException; @@ -24,6 +27,17 @@ public class SentryInitBeanPostProcessor implements BeanPostProcessor, ApplicationContextAware, DisposableBean { private @Nullable ApplicationContext applicationContext; + private final @NotNull IHub hub; + + public SentryInitBeanPostProcessor() { + this(HubAdapter.getInstance()); + } + + SentryInitBeanPostProcessor(final @NotNull IHub hub) { + Objects.requireNonNull(hub, "hub is required"); + this.hub = hub; + } + @Override @SuppressWarnings({"unchecked", "deprecation"}) public @NotNull Object postProcessAfterInitialization( @@ -76,6 +90,6 @@ public void setApplicationContext(final @NotNull ApplicationContext applicationC @Override public void destroy() { - Sentry.close(); + hub.close(); } } diff --git a/sentry-spring/src/test/kotlin/io/sentry/spring/SentryInitBeanPostProcessorTest.kt b/sentry-spring/src/test/kotlin/io/sentry/spring/SentryInitBeanPostProcessorTest.kt new file mode 100644 index 00000000000..e2213939352 --- /dev/null +++ b/sentry-spring/src/test/kotlin/io/sentry/spring/SentryInitBeanPostProcessorTest.kt @@ -0,0 +1,30 @@ +package io.sentry.spring + +import com.nhaarman.mockitokotlin2.mock +import com.nhaarman.mockitokotlin2.verify +import io.sentry.IHub +import org.springframework.context.annotation.AnnotationConfigApplicationContext +import org.springframework.context.annotation.Bean +import org.springframework.context.annotation.Configuration +import kotlin.test.Test + +class SentryInitBeanPostProcessorTest { + + @Test + fun closesSentryOnApplicationContextDestroy() { + val ctx = AnnotationConfigApplicationContext(TestConfig::class.java) + val hub = ctx.getBean(IHub::class.java) + ctx.close() + verify(hub).close() + } + + @Configuration + open class TestConfig { + + @Bean(destroyMethod = "") + open fun hub() = mock() + + @Bean + open fun sentryInitBeanPostProcessor() = SentryInitBeanPostProcessor(hub()) + } +}