ThreadLocal Hubs should be cleaned up

[adinauer]

Problem Statement

At the moment Hub instances are stored in ThreadLocal variables. They are never cleaned up and thus cause servers like Tomcat to issue warnings as can be seen here #2074, here #420 and here #487. While there are mechanisms to clean up these leaks in Tomcat we should not rely on them and clean up instead.

sentry-java/sentry/src/main/java/io/sentry/Sentry.java

Line 21 in 9f1af1c

private static final @NotNull ThreadLocal<IHub> currentHub = new ThreadLocal<>();

Solution Brainstorm

We may have to move away from ThreadLocal variables at some point to support Webflux and similar. Until then we could try and unset the ThreadLocal after a request is done (ServletRequestListener.requestDestroyed)

[bruno-garcia]

Closed through #2076 (somehow GitHub didn't automatically close it)

[adinauer]

#2076 only allows Threads that had any Sentry.* method called before Sentry.init to self heal. This issue is still unresolved.

[adinauer]

We could do it similar to this: sentry/src/main/java/io/sentry/servlet/SentryServletRequestListener.java

[adamnegaard]

Is there any way to avoid the memory leaks in tomcat 9 caused by this issue? It's is causing developers to have to restart tomcat whenever we have redeployed applications a few times.
If any one could give me a tip on how to get started fixing this issue, i might have time to try and solve it , assuming it's a small effort.

[adinauer]

Hello @adamnegaard what frameworks are you using? Is it Spring? Spring Boot? WebMVC? WebFlux?

Basically the idea here is to call Sentry.setCurrentHub(NoOpHub.getInstance()); once we know we're done with a request. In the latest versions of the SDK this should already be the case for WebFlux. This should cleanup the hubs created on the request threads. However there may still be more thread locals depending on thread / SDK usage.

[adamnegaard]

Hi @adinauer, thanks for taking the time to get back to me.
We are using Spring Boot, for developing API's. We build our applications as .WAR files, and deploy them on a tomcat server. We are using logback for logging, and thus using the SentryAppender found here:
https://github.com/getsentry/sentry-java/blob/a78496a9730742d6837339f2b359e66776f94eb1/sentry-logback/src/main/java/io/sentry/logback/SentryAppender.java

Whenever we undeploy an app, tomcat is not able to unload classes, which have been loaded by the app. It seems like the reason for this is the static reference to a ThreadLocal here, which cannot be terminated on application undeploy:
https://github.com/getsentry/sentry-java/blob/a78496a9730742d6837339f2b359e66776f94eb1/sentry/src/main/java/io/sentry/Sentry.java#LL33C10-L33C10
On every reploy, tomcat loads all the needed classes for the app, but never unloads the classes from the undeployed application. This essentially causes a PermGen memory leak.
It is worth mentioning, whenever we restart tomcat, it is able to unload all the classes, but this is not ideal for our current setup.

What i have tried so far, is creating a CustomSentryAppender that extends SentryAppender, and using that with logback. I have tried overriding the stop() method as follows:

@Override
public void stop() {
    Sentry.setCurrentHub(NoOpHub.getInstance());
    Sentry.close();
    super.stop();
}

I can see that this method gets invoked on undeploy, but it does not seem to be fixing our memory leak.

Is there any known workaround for this issue? Otherwise, could you give me any pointers to where Sentry requests are performed, so i can try unsetting the hub reference after a request is done?
Thanks a lot 😄 .

[adinauer]

@adamnegaard the problem here is that we set the thread local on every thread where interaction with the Sentry SDK happens. So basically on undeploy you'd have to go to every thread and unset the current hub there as there's no way to unset from another thread. The appenders stop method is probably called on one thread whereas hub instances are bound to different threads thread local variables.

I'm not entirely sure how this works in Tomcat but my guess is there's only a few threads that survive a redeploy and those would have to be cleaned up. Not sure you can register some callback that is invoked on each of those threads to then unset the hub.

Is there any known workaround for this issue?

Unfortunately the current workaround is restarting Tomcat.

You could try to do the following:

• Define a custom SentrySpringFilter bean to replace the original one
• Replace the doFilterInternal method with this:

  @Override
  protected void doFilterInternal(
      final @NotNull HttpServletRequest servletRequest,
      final @NotNull HttpServletResponse response,
      final @NotNull FilterChain filterChain)
      throws ServletException, IOException {
    IHub hub = Sentry.cloneMainHub();
    if (hub.isEnabled()) {
      // request may qualify for caching request body, if so resolve cached request
      final HttpServletRequest request = resolveHttpServletRequest(servletRequest);
//      hub.pushScope();
      try {
        final Hint hint = new Hint();
        hint.set(SPRING_REQUEST_FILTER_REQUEST, servletRequest);
        hint.set(SPRING_REQUEST_FILTER_RESPONSE, response);

        hub.addBreadcrumb(Breadcrumb.http(request.getRequestURI(), request.getMethod()), hint);
        configureScope(request);
        filterChain.doFilter(request, response);
      } finally {
//        hub.popScope();
        Sentry.setCurrentHub(NoOpHub.getInstance());
      }
    } else {
      filterChain.doFilter(servletRequest, response);
    }
  }

This is what we do for WebFlux. So far there's no complaints for that approach so we could change WebMVC to do the same, i.e. clone a new hub when the request starts and unset the hub when it finishes instead of calling pushScope and popScope. This may not get rid of all the thread local variables if you're using multiple threads, e.g. async etc.

If you decide to give this a try, please leave some feedback 🙏

pointers to where Sentry requests are performed, so i can try unsetting the hub reference after a request is done?

It's not just requests to Sentry. It's any request that comes into your server that then likely creates a hub instances and binds it to the current threads thread local variable for every thread that is used during the request.

[adamnegaard]

@adinauer I did try this approach, but does not seem to solve the problem. A IHub is being created in many different threads which I do not directly have control over, for example in Spring @Scheduled jobs.

It seems like the best solution would be moving away from ThreadLocals, since Spring in general creates threads behind the scenes.
With the current setup, each thread that has created a hub on the ThreadLocal class variable would have to call remove() on the ThreadLocal. This seems to be very difficult when threads are created and managed by Spring. Simply removing the reference to a hub, i.e. Sentry.setCurrentHub(NoOpHub.getInstance()); is not enough, for tomcat to be able to shutdown the application, remove() needs to be called on the ThreadLocal (which can only be done through Sentry.close()).
A possible solution might be having a ConcurrentMap that stores the IHubs. On Sentry.close(), it would still be possible to close only the hub from the calling thread. Although tomcat should be able to GC the IHubs.

[adamnegaard]

@adinauer I created a PR #2711 with the changes described above. Would you or any of the other maintainers mind looking at it, and getting back with feedback if any?

Thanks in advance

[adinauer]

Hello @adamnegaard have you tried this locally? Does it fix the problem?

I've tried something very similar before and ran into multiple issues (need to check where I wrote them down). This requires lots of testing. Not sure when I'll get to that - apologies.

[adamnegaard]

Sorry for getting back so late to this. I have not testet it locally. Do you have any tips on how the best way to that might be? Also wondering if there are any other updates to this issue, that might be worth looking into?

[adinauer]

Hey, I replied on #2711 (comment)