diff --git a/changelog/@unreleased/pr-1912.v2.yml b/changelog/@unreleased/pr-1912.v2.yml new file mode 100644 index 000000000..2dc0b1666 --- /dev/null +++ b/changelog/@unreleased/pr-1912.v2.yml @@ -0,0 +1,5 @@ +type: improvement +improvement: + description: Dialogue attempts not to retain context classloaders + links: + - https://github.com/palantir/dialogue/pull/1912 diff --git a/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/CleanerSupport.java b/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/CleanerSupport.java index 0f5691e8a..379197fce 100644 --- a/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/CleanerSupport.java +++ b/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/CleanerSupport.java @@ -16,17 +16,15 @@ package com.palantir.dialogue.hc5; -import com.google.common.util.concurrent.ThreadFactoryBuilder; import java.lang.ref.Cleaner; import java.lang.ref.Cleaner.Cleanable; /** Reflective shim to allow consumers on new runtime versions to take advantage of java.lang.ref.Cleaner. */ final class CleanerSupport { - private static final Cleaner cleaner = Cleaner.create(new ThreadFactoryBuilder() - .setDaemon(true) - .setNameFormat("dialogue-cleaner-%d") - .build()); + // Ideally we would use a name pattern like 'dialogue-cleaner-%d', however it's more important + // that this cleaner does not retain context classloader references. + private static final Cleaner cleaner = Cleaner.create(); /** Arguments are passed to {@code java.lang.ref.Cleaner.register(Object, Runnable)}. */ static Cleanable register(Object object, Runnable action) { diff --git a/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/ScheduledIdleConnectionEvictor.java b/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/ScheduledIdleConnectionEvictor.java index 4ea71e1c5..e2837378d 100644 --- a/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/ScheduledIdleConnectionEvictor.java +++ b/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/ScheduledIdleConnectionEvictor.java @@ -17,7 +17,6 @@ package com.palantir.dialogue.hc5; import com.google.common.base.Suppliers; -import com.google.common.util.concurrent.ThreadFactoryBuilder; import com.palantir.dialogue.core.DialogueExecutors; import com.palantir.logsafe.logger.SafeLogger; import com.palantir.logsafe.logger.SafeLoggerFactory; @@ -46,10 +45,7 @@ final class ScheduledIdleConnectionEvictor { SharedTaggedMetricRegistries.getSingleton(), DialogueExecutors.newSharedSingleThreadScheduler(MetricRegistries.instrument( SharedTaggedMetricRegistries.getSingleton(), - new ThreadFactoryBuilder() - .setNameFormat(EXECUTOR_NAME + "-%d") - .setDaemon(true) - .build(), + DialogueExecutors.newDaemonThreadFactory(EXECUTOR_NAME), EXECUTOR_NAME)), EXECUTOR_NAME)); diff --git a/dialogue-blocking-channels/build.gradle b/dialogue-blocking-channels/build.gradle index 1013369fc..5037e3f6f 100644 --- a/dialogue-blocking-channels/build.gradle +++ b/dialogue-blocking-channels/build.gradle @@ -5,6 +5,7 @@ dependencies { api project(':dialogue-target') api 'com.google.guava:guava' implementation project(':dialogue-futures') + implementation project(':dialogue-core') implementation 'com.palantir.tracing:tracing' implementation 'com.palantir.tritium:tritium-metrics' implementation 'com.palantir.safe-logging:logger' diff --git a/dialogue-blocking-channels/src/main/java/com/palantir/dialogue/blocking/BlockingChannelAdapter.java b/dialogue-blocking-channels/src/main/java/com/palantir/dialogue/blocking/BlockingChannelAdapter.java index f8e885a04..389b38541 100644 --- a/dialogue-blocking-channels/src/main/java/com/palantir/dialogue/blocking/BlockingChannelAdapter.java +++ b/dialogue-blocking-channels/src/main/java/com/palantir/dialogue/blocking/BlockingChannelAdapter.java @@ -20,11 +20,11 @@ import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.SettableFuture; -import com.google.common.util.concurrent.ThreadFactoryBuilder; import com.palantir.dialogue.Channel; import com.palantir.dialogue.Endpoint; import com.palantir.dialogue.Request; import com.palantir.dialogue.Response; +import com.palantir.dialogue.core.DialogueExecutors; import com.palantir.dialogue.futures.DialogueFutures; import com.palantir.logsafe.SafeArg; import com.palantir.logsafe.logger.SafeLogger; @@ -42,17 +42,15 @@ public final class BlockingChannelAdapter { private static final SafeLogger log = SafeLoggerFactory.get(BlockingChannelAdapter.class); + private static final String EXECUTOR_NAME = "dialogue-blocking-channel"; @SuppressWarnings("deprecation") // No reasonable way to pass a tagged registry to this singleton private static final Supplier blockingExecutor = Suppliers.memoize(() -> Tracers.wrap( - "dialogue-blocking-channel", + EXECUTOR_NAME, Executors.newCachedThreadPool(MetricRegistries.instrument( SharedTaggedMetricRegistries.getSingleton(), - new ThreadFactoryBuilder() - .setNameFormat("dialogue-blocking-channel-%d") - .setDaemon(true) - .build(), - "dialogue-blocking-channel")))); + DialogueExecutors.newDaemonThreadFactory(EXECUTOR_NAME), + EXECUTOR_NAME)))); public static Channel of(BlockingChannel blockingChannel) { return of(blockingChannel, blockingExecutor.get()); diff --git a/dialogue-core/src/main/java/com/palantir/dialogue/core/DialogueExecutors.java b/dialogue-core/src/main/java/com/palantir/dialogue/core/DialogueExecutors.java index 2e02c8448..d059df68e 100644 --- a/dialogue-core/src/main/java/com/palantir/dialogue/core/DialogueExecutors.java +++ b/dialogue-core/src/main/java/com/palantir/dialogue/core/DialogueExecutors.java @@ -17,6 +17,9 @@ package com.palantir.dialogue.core; import com.google.common.annotations.VisibleForTesting; +import com.google.common.util.concurrent.ThreadFactoryBuilder; +import java.security.AccessController; +import java.security.PrivilegedAction; import java.time.Duration; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ScheduledThreadPoolExecutor; @@ -52,5 +55,22 @@ static ScheduledExecutorService newSharedSingleThreadScheduler( return executor; } + /** + * Create a new ThreadFactory which creates threads that do not retain a ContextClassLoader reference. + */ + public static ThreadFactory newDaemonThreadFactory(String prefix) { + ThreadFactory delegate = new ThreadFactoryBuilder() + .setNameFormat(prefix + "-%d") + .setDaemon(true) + .build(); + return runnable -> { + Thread thread = delegate.newThread(runnable); + return AccessController.doPrivileged((PrivilegedAction) () -> { + thread.setContextClassLoader(null); + return thread; + }); + }; + } + private DialogueExecutors() {} } diff --git a/dialogue-core/src/main/java/com/palantir/dialogue/core/RetryingChannel.java b/dialogue-core/src/main/java/com/palantir/dialogue/core/RetryingChannel.java index 6dbcae3b8..0c41d6ff6 100644 --- a/dialogue-core/src/main/java/com/palantir/dialogue/core/RetryingChannel.java +++ b/dialogue-core/src/main/java/com/palantir/dialogue/core/RetryingChannel.java @@ -25,7 +25,6 @@ import com.google.common.util.concurrent.ListeningScheduledExecutorService; import com.google.common.util.concurrent.MoreExecutors; import com.google.common.util.concurrent.SettableFuture; -import com.google.common.util.concurrent.ThreadFactoryBuilder; import com.palantir.conjure.java.client.config.ClientConfiguration; import com.palantir.dialogue.Endpoint; import com.palantir.dialogue.EndpointChannel; @@ -76,10 +75,7 @@ final class RetryingChannel implements EndpointChannel { static final Supplier sharedScheduler = Suppliers.memoize(() -> DialogueExecutors.newSharedSingleThreadScheduler(MetricRegistries.instrument( SharedTaggedMetricRegistries.getSingleton(), - new ThreadFactoryBuilder() - .setNameFormat(SCHEDULER_NAME + "-%d") - .setDaemon(true) - .build(), + DialogueExecutors.newDaemonThreadFactory(SCHEDULER_NAME), SCHEDULER_NAME))); @SuppressWarnings("UnnecessaryLambda") // no allocations