Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Dialogue attempts not to retain context classloaders #1912

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from
Draft

Dialogue attempts not to retain context classloaders #1912

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b17df92
Dialogue attempts not to retain context classloaders
carterkozak Apr 18, 2023
f2a2e51
Add generated changelog entries
svc-changelog Apr 18, 2023
1c64ebc
Autorelease 3.80.0-rc1
carterkozak Apr 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions changelog/@unreleased/pr-1912.v2.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
type: improvement
improvement:
description: Dialogue attempts not to retain context classloaders
links:
- https://github.com/palantir/dialogue/pull/1912
8 changes: 3 additions & 5 deletions dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/CleanerSupport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could not use ThreadFactoryBuilder and impl from scratch?

Copy link
Contributor Author

@carterkozak carterkozak Apr 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite, cleaner has some special case logic to clear threadlocal data after cleaning objects when the thread implementation is the internal default (InnocuousThread) here. Unfortunately it becomes complex to duplicate that logic (requires Unsafe apis)


/** Arguments are passed to {@code java.lang.ref.Cleaner.register(Object, Runnable)}. */
static Cleanable register(Object object, Runnable action) {
Expand Down
6 changes: 1 addition & 5 deletions ...he-hc5-client/src/main/java/com/palantir/dialogue/hc5/ScheduledIdleConnectionEvictor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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));

Expand Down
1 change: 1 addition & 0 deletions dialogue-blocking-channels/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
12 changes: 5 additions & 7 deletions ...locking-channels/src/main/java/com/palantir/dialogue/blocking/BlockingChannelAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<ExecutorService> 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());
Expand Down
20 changes: 20 additions & 0 deletions dialogue-core/src/main/java/com/palantir/dialogue/core/DialogueExecutors.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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>) () -> {
thread.setContextClassLoader(null);
return thread;
});
};
}

private DialogueExecutors() {}
}
6 changes: 1 addition & 5 deletions dialogue-core/src/main/java/com/palantir/dialogue/core/RetryingChannel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -76,10 +75,7 @@ final class RetryingChannel implements EndpointChannel {
static final Supplier<ScheduledExecutorService> 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
Expand Down