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.

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

Hubs/Scopes Merge 15 - Replace ThreadLocal with scope storage #3317

Merged
merged 19 commits into from
Apr 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
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
24 changes: 24 additions & 0 deletions sentry/api/sentry.api
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,13 @@ public final class io/sentry/DeduplicateMultithreadedEventProcessor : io/sentry/
public fun process (Lio/sentry/SentryEvent;Lio/sentry/Hint;)Lio/sentry/SentryEvent;
}

public final class io/sentry/DefaultScopesStorage : io/sentry/IScopesStorage {
public fun <init> ()V
public fun close ()V
public fun get ()Lio/sentry/IScopes;
public fun set (Lio/sentry/IScopes;)Lio/sentry/ISentryLifecycleToken;
}

public final class io/sentry/DefaultTransactionPerformanceCollector : io/sentry/TransactionPerformanceCollector {
public fun <init> (Lio/sentry/SentryOptions;)V
public fun close ()V
Expand Down Expand Up @@ -792,6 +799,12 @@ public abstract interface class io/sentry/IScopes {
public abstract fun withScope (Lio/sentry/ScopeCallback;)V
}

public abstract interface class io/sentry/IScopesStorage {
public abstract fun close ()V
public abstract fun get ()Lio/sentry/IScopes;
public abstract fun set (Lio/sentry/IScopes;)Lio/sentry/ISentryLifecycleToken;
}

public abstract interface class io/sentry/ISentryClient {
public abstract fun captureCheckIn (Lio/sentry/CheckIn;Lio/sentry/IScope;Lio/sentry/Hint;)Lio/sentry/protocol/SentryId;
public fun captureEnvelope (Lio/sentry/SentryEnvelope;)Lio/sentry/protocol/SentryId;
Expand Down Expand Up @@ -831,6 +844,10 @@ public abstract interface class io/sentry/ISentryExecutorService {
public abstract fun submit (Ljava/util/concurrent/Callable;)Ljava/util/concurrent/Future;
}

public abstract interface class io/sentry/ISentryLifecycleToken : java/lang/AutoCloseable {
public abstract fun close ()V
}

public abstract interface class io/sentry/ISerializer {
public abstract fun deserialize (Ljava/io/Reader;Ljava/lang/Class;)Ljava/lang/Object;
public abstract fun deserializeCollection (Ljava/io/Reader;Ljava/lang/Class;Lio/sentry/JsonDeserializer;)Ljava/lang/Object;
Expand Down Expand Up @@ -1390,6 +1407,13 @@ public final class io/sentry/NoOpScopes : io/sentry/IScopes {
public fun withScope (Lio/sentry/ScopeCallback;)V
}

public final class io/sentry/NoOpScopesStorage : io/sentry/IScopesStorage {
public fun close ()V
public fun get ()Lio/sentry/IScopes;
public static fun getInstance ()Lio/sentry/NoOpScopesStorage;
public fun set (Lio/sentry/IScopes;)Lio/sentry/ISentryLifecycleToken;
}

public final class io/sentry/NoOpSpan : io/sentry/ISpan {
public fun finish ()V
public fun finish (Lio/sentry/SpanStatus;)V
Expand Down
42 changes: 42 additions & 0 deletions sentry/src/main/java/io/sentry/DefaultScopesStorage.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
package io.sentry;

import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;

public final class DefaultScopesStorage implements IScopesStorage {

private static final @NotNull ThreadLocal<IScopes> currentScopes = new ThreadLocal<>();

@Override
public ISentryLifecycleToken set(@Nullable IScopes scopes) {
final @Nullable IScopes oldScopes = get();
currentScopes.set(scopes);
return new DefaultScopesLifecycleToken(oldScopes);
}

@Override
public @Nullable IScopes get() {
return currentScopes.get();
}

@Override
public void close() {
// TODO prevent further storing? would this cause problems if singleton, closed and
// re-initialized?
currentScopes.remove();
}

static final class DefaultScopesLifecycleToken implements ISentryLifecycleToken {

private final @Nullable IScopes oldValue;

DefaultScopesLifecycleToken(final @Nullable IScopes scopes) {
this.oldValue = scopes;
}

@Override
public void close() {
currentScopes.set(oldValue);
}
}
}
13 changes: 13 additions & 0 deletions sentry/src/main/java/io/sentry/IScopesStorage.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package io.sentry;

import org.jetbrains.annotations.Nullable;

public interface IScopesStorage {

ISentryLifecycleToken set(final @Nullable IScopes scopes);
adinauer marked this conversation as resolved.
Show resolved Hide resolved

@Nullable
IScopes get();

void close();
}
8 changes: 8 additions & 0 deletions sentry/src/main/java/io/sentry/ISentryLifecycleToken.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package io.sentry;

public interface ISentryLifecycleToken extends AutoCloseable {

// overridden to not have a checked exception on the method.
@Override
void close();
}
40 changes: 40 additions & 0 deletions sentry/src/main/java/io/sentry/NoOpScopesStorage.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package io.sentry;

import org.jetbrains.annotations.Nullable;

public final class NoOpScopesStorage implements IScopesStorage {
private static final NoOpScopesStorage instance = new NoOpScopesStorage();

private NoOpScopesStorage() {}

public static NoOpScopesStorage getInstance() {
return instance;
}

@Override
public ISentryLifecycleToken set(@Nullable IScopes scopes) {
return NoOpScopesLifecycleToken.getInstance();
}

@Override
public @Nullable IScopes get() {
return NoOpScopes.getInstance();
}

@Override
public void close() {}

static final class NoOpScopesLifecycleToken implements ISentryLifecycleToken {

private static final NoOpScopesLifecycleToken instance = new NoOpScopesLifecycleToken();

private NoOpScopesLifecycleToken() {}

public static NoOpScopesLifecycleToken getInstance() {
return instance;
}

@Override
public void close() {}
}
}
42 changes: 25 additions & 17 deletions sentry/src/main/java/io/sentry/Sentry.java
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,7 @@ public final class Sentry {

private Sentry() {}

/** Holds Hubs per thread or only mainScopes if globalHubMode is enabled. */
private static final @NotNull ThreadLocal<IScopes> currentScopes = new ThreadLocal<>();
private static volatile @NotNull IScopesStorage scopesStorage = new DefaultScopesStorage();

/** The Main Hub or NoOp if Sentry is disabled. */
private static volatile @NotNull IScopes mainScopes = NoOpScopes.getInstance();
Expand Down Expand Up @@ -83,13 +82,17 @@ private Sentry() {}
if (globalHubMode) {
return mainScopes;
}
IScopes hub = currentScopes.get();
if (hub == null || hub.isNoOp()) {
IScopes scopes = getScopesStorage().get();
if (scopes == null || scopes.isNoOp()) {
// TODO fork instead
hub = mainScopes.clone();
currentScopes.set(hub);
scopes = mainScopes.clone();
getScopesStorage().set(scopes);
}
return hub;
return scopes;
}

private static @NotNull IScopesStorage getScopesStorage() {
return scopesStorage;
}

/**
Expand All @@ -110,14 +113,15 @@ private Sentry() {}

@ApiStatus.Internal // exposed for the coroutines integration in SentryContext
@Deprecated
@SuppressWarnings("deprecation")
@SuppressWarnings({"deprecation", "InlineMeSuggester"})
public static void setCurrentHub(final @NotNull IHub hub) {
currentScopes.set(hub);
setCurrentScopes(hub);
}

@ApiStatus.Internal // exposed for the coroutines integration in SentryContext
public static void setCurrentScopes(final @NotNull IScopes scopes) {
currentScopes.set(scopes);
public static @NotNull ISentryLifecycleToken setCurrentScopes(final @NotNull IScopes scopes) {
return getScopesStorage().set(scopes);
}
adinauer marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down Expand Up @@ -253,14 +257,18 @@ private static synchronized void init(
options.getLogger().log(SentryLevel.INFO, "GlobalHubMode: '%s'", String.valueOf(globalHubMode));
Sentry.globalHubMode = globalHubMode;

final IScopes hub = getCurrentScopes();
final IScopes scopes = getCurrentScopes();
final IScope rootScope = new Scope(options);
final IScope rootIsolationScope = new Scope(options);
mainScopes = new Scopes(rootScope, rootIsolationScope, options, "Sentry.init");
// TODO should use separate isolation scope:
// final IScope rootIsolationScope = new Scope(options);
// TODO should be:
// getGlobalScope().bindClient(new SentryClient(options));
rootScope.bindClient(new SentryClient(options));
mainScopes = new Scopes(rootScope, rootScope, options, "Sentry.init");

currentScopes.set(mainScopes);
getScopesStorage().set(mainScopes);

hub.close(true);
scopes.close(true);

// If the executorService passed in the init is the same that was previously closed, we have to
// set a new one
Expand Down Expand Up @@ -508,7 +516,7 @@ public static synchronized void close() {
final IScopes scopes = getCurrentScopes();
mainScopes = NoOpScopes.getInstance();
// remove thread local to avoid memory leak
currentScopes.remove();
getScopesStorage().close();
Copy link
Collaborator

Choose a reason for hiding this comment

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

If not in globalHubMode, is it enough to clean the ThreadLocal in the calling thread?
What about scopes stored in other long-living threads, like worker threads or threads in ThreadPools that are reused, wouldn't the scopes in there live on? Would have been the same for Hub before the Hubs/Scopes merge.

If one would re-init Sentry, the ThreadLocal on the calling thread is cleaned, if, however there were long-living threads that had a reference to the initial Scopes instance, they would still use the initial version of Scopes when calling Sentry.captureX for example.

Not sure about the implications, just wanted to point out a potential issue.

Please correct me if I'm wrong :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, maybe we can give https://github.com/getsentry/sentry-java/pull/2711/files a try once we're done with most changes for the major.

scopes.close(false);
}

Expand Down
Loading