Possible memory leak on application redeploy on Tomcat 8.0

[foal]

Tomcat 8.0
sentry-logback 1.4.0
buffer.gracefulshutdown=false
async.gracefulshutdown=false

13-Aug-2017 11:29:09.808 INFO [http-nio-6080-exec-41] org.apache.catalina.startup.HostConfig.undeploy Undeploying context [/ftadmin]
13-Aug-2017 11:29:10.075 SEVERE [http-nio-6080-exec-41] org.apache.catalina.loader.WebappClassLoaderBase.checkThreadLocalMapForLeaks The web application [ftadmin] created a ThreadLocal with key of type [io.sentry.environment.SentryEnvironment$1] (value [io.sentry.environment.SentryEnvironment$1@26c761df]) and a value of type [java.util.concurrent.atomic.AtomicInteger] (value [0]) but failed to remove it when the web application was stopped. Threads are going to be renewed over time to try and avoid a probable memory leak.
13-Aug-2017 11:29:10.075 SEVERE [http-nio-6080-exec-41] org.apache.catalina.loader.WebappClassLoaderBase.checkThreadLocalMapForLeaks The web application [ftadmin] created a ThreadLocal with key of type [io.sentry.environment.SentryEnvironment$1] (value [io.sentry.environment.SentryEnvironment$1@26c761df]) and a value of type [java.util.concurrent.atomic.AtomicInteger] (value [0]) but failed to remove it when the web application was stopped. Threads are going to be renewed over time to try and avoid a probable memory leak.
13-Aug-2017 11:29:10.076 SEVERE [http-nio-6080-exec-41] org.apache.catalina.loader.WebappClassLoaderBase.checkThreadLocalMapForLeaks The web application [ftadmin] created a ThreadLocal with key of type [io.sentry.environment.SentryEnvironment$1] (value [io.sentry.environment.SentryEnvironment$1@26c761df]) and a value of type [java.util.concurrent.atomic.AtomicInteger] (value [0]) but failed to remove it when the web application was stopped. Threads are going to be renewed over time to try and avoid a probable memory leak.
13-Aug-2017 11:29:10.076 SEVERE [http-nio-6080-exec-41] org.apache.catalina.loader.WebappClassLoaderBase.checkThreadLocalMapForLeaks The web application [ftadmin] created a ThreadLocal with key of type [io.sentry.environment.SentryEnvironment$1] (value [io.sentry.environment.SentryEnvironment$1@26c761df]) and a value of type [java.util.concurrent.atomic.AtomicInteger] (value [0]) but failed to remove it when the web application was stopped. Threads are going to be renewed over time to try and avoid a probable memory leak.
13-Aug-2017 11:29:10.076 SEVERE [http-nio-6080-exec-41] org.apache.catalina.loader.WebappClassLoaderBase.checkThreadLocalMapForLeaks The web application [ftadmin] created a ThreadLocal with key of type [io.sentry.environment.SentryEnvironment$1] (value [io.sentry.environment.SentryEnvironment$1@26c761df]) and a value of type [java.util.concurrent.atomic.AtomicInteger] (value [0]) but failed to remove it when the web application was stopped. Threads are going to be renewed over time to try and avoid a probable memory leak.

[bretthoerner]

Hmm, seems like SentryEnvironment should really be per-client and not static (to fix this and for other reasons).

That said, Tomcat loves to warn about these things even though it actually does handle it,

Threads are going to be renewed over time to try and avoid a probable memory leak.

And it's a single AtomicInteger until the thread is refreshed, so I doubt it's going to effect anyone in production. But I will see about how to fix this, thanks for the report.

[bretthoerner]

Or, another open question is: do we actually need SentryEnvironment at all? 🤔

[sbezkostnyi]

I've this issue too.

It seems that you can register ServletContextListener in SentryServletContainerInitializer that will cleanup ThreadLocal in SentryEnvironment.

[bretthoerner]

And that won't affect other applications in Tomcat that might be using Sentry? I'm not a Tomcat expert, will they have their own copies of SentryEnvironment even though it's static?

[sbezkostnyi]

Technically they should, since Tomcat uses separate class loaders for each application.

But I was wrong, we can't simply cleanup ThreadLocal for all threads, we can do this only for each thread separately and only from inside of the thread.

So, I've another suggestion.

In SentryEnvironment.stopManagingThread() we can replace:

SENTRY_THREAD.get().decrementAndGet();

with something like this:

if (SENTRY_THREAD.get().decrementAndGet() == 0) {
    SENTRY_THREAD.remove();
}

Since value is zero, we can remove it safely, next call to SENTRY_THREAD.get() will initialize it again.

This way if amount of calls to SentryEnvironment.startManagingThread() matches SentryEnvironment.stopManagingThread(), thread local value will be absent on shutdown.

[bretthoerner]

@sbezkostnyi Good idea! Thanks, I'll try that out.