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

Remove usage of ThreadLocal for IHubs #2711

Closed
wants to merge 2 commits into from
Closed

Remove usage of ThreadLocal for IHubs #2711

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9a031a5
Remove usage of ThreadLocal for IHubs. Store IHubs for each thread in…
adamnegaard May 15, 2023
890e17b
Update comments in SentryTest.kt reflecting the new change for storin…
adamnegaard May 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 17 additions & 9 deletions sentry/src/main/java/io/sentry/Sentry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
import java.lang.reflect.InvocationTargetException;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.RejectedExecutionException;
import org.jetbrains.annotations.ApiStatus;
import org.jetbrains.annotations.NotNull;
Expand All @@ -30,7 +32,7 @@ public final class Sentry {
private Sentry() {}

/** Holds Hubs per thread or only mainHub if globalHubMode is enabled. */
private static final @NotNull ThreadLocal<IHub> currentHub = new ThreadLocal<>();
private static final @NotNull Map<Long, IHub> hubMap = new ConcurrentHashMap<>();

/** The Main Hub or NoOp if Sentry is disabled. */
private static volatile @NotNull IHub mainHub = NoOpHub.getInstance();
Expand All @@ -51,10 +53,11 @@ private Sentry() {}
if (globalHubMode) {
return mainHub;
}
IHub hub = currentHub.get();
Thread currentThread = Thread.currentThread();
IHub hub = hubMap.get(currentThread.getId());
if (hub == null || hub instanceof NoOpHub) {
hub = mainHub.clone();
currentHub.set(hub);
hubMap.put(currentThread.getId(), hub);
}
return hub;
}
Expand All @@ -75,7 +78,7 @@ private Sentry() {}

@ApiStatus.Internal // exposed for the coroutines integration in SentryContext
public static void setCurrentHub(final @NotNull IHub hub) {
currentHub.set(hub);
hubMap.put(Thread.currentThread().getId(), hub);
}

/**
Expand Down Expand Up @@ -213,7 +216,7 @@ private static synchronized void init(
final IHub hub = getCurrentHub();
mainHub = new Hub(options);

currentHub.set(mainHub);
hubMap.put(Thread.currentThread().getId(), mainHub);

hub.close();

Expand Down Expand Up @@ -378,11 +381,16 @@ private static boolean initConfigurations(final @NotNull SentryOptions options)

/** Close the SDK */
public static synchronized void close() {
final IHub hub = getCurrentHub();
mainHub = NoOpHub.getInstance();
// remove thread local to avoid memory leak
currentHub.remove();
hub.close();
// close the current threads hub
Thread currentThread = Thread.currentThread();
IHub hub = hubMap.get(currentThread.getId());
if (hub != null) {
hub.close();
// remove the current threads hub from the map
hubMap.remove(currentThread.getId());
}
Copy link
Member

Choose a reason for hiding this comment

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

In case we come back to this PR to give it a try in the next major, we should also clear the map on close.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry for being so slow on this. I have not run this in production yet. I'm a bit sure about the best way to go about testing this. Do you have any tips for me as to how i could test it, to be enough to be included in a future release? We are still dealing with this issue in production, but are just managing with manual restarts of tomcat now and then.

Copy link
Member

Choose a reason for hiding this comment

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

We're in the middle of rewriting some parts of the SDK for the upcoming 8.x release so the PR won't be mergable as is. It should be rather straight forward to adapt it to the new IScopesStorage interface.

Do you have any tips for me as to how i could test it

  • Basically spin up the application before and after the change and compare what lands in Sentry.
    • Keep an eye out for data that's missing
    • Also check for data that doesn't belong to errors/transactions/... you're comparing as they might indicate leaks
  • Spin up the server locally, hit it with lots of requests, run GC a couple times then capture a thread dump
    • You can compare the results of before and after the change
    • There shouldn't be any additional objects being held on to (Hub, Scope, Attachment, Breadcrumb, tags etc.)
  • Check whether your tomcat warnings go away when using the updated version

As this affects all of the SDK there is a lot more testing required that's why we haven't yet given this PR the love it deserves.

In 8.x of the Java SDK we'll have to retest whether this issue even exists anymore since we're introducing lifecycle tokens that you can call close on when you're done so maybe that already takes care of cleaning up ThreadLocals. I haven't gotten around to testing this yet.


}

/**
Expand Down
6 changes: 3 additions & 3 deletions sentry/src/test/java/io/sentry/SentryTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ class SentryTest {

@Test
fun `using sentry before calling init creates NoOpHub but after init Sentry uses a new clone`() {
// noop as not yet initialized, caches NoOpHub in ThreadLocal
// noop as not yet initialized, caches NoOpHub in hubMap
Sentry.captureMessage("noop caused")

assertTrue(Sentry.getCurrentHub() is NoOpHub)
Expand All @@ -260,7 +260,7 @@ class SentryTest {

@Test
fun `main hub can be cloned and does not share scope with current hub`() {
// noop as not yet initialized, caches NoOpHub in ThreadLocal
// noop as not yet initialized, caches NoOpHub in hubMap
Sentry.addBreadcrumb("breadcrumbNoOp")
Sentry.captureMessage("messageNoOp")

Expand Down Expand Up @@ -311,7 +311,7 @@ class SentryTest {

@Test
fun `main hub is not cloned in global hub mode and shares scope with current hub`() {
// noop as not yet initialized, caches NoOpHub in ThreadLocal
// noop as not yet initialized, caches NoOpHub in hubMap
Sentry.addBreadcrumb("breadcrumbNoOp")
Sentry.captureMessage("messageNoOp")

Expand Down