search

kamon-io / kanela

The Kamon Instrumentation Agent
Apache License 2.0
68 stars 22 forks source link

Memory leak in tinylog #135

Closed jtjeferreira closed 2 years ago

jtjeferreira commented 2 years ago

When instrumentation is triggering errors like the below:

An error occurred while trying to apply an advisor: java.lang.NoClassDefFoundError: redis/clients/jedis/commands/ProtocolCommand

These errors are not printed to the console, and instead are causing an memory leak.

I found this via an heapdump: image

I think the problem is that the writing thread of tinylog is no longer running, becase it is configured to track the "main" thread and stops as soon the "main" thread dies...

https://github.com/kamon-io/kanela/blob/f7b7dc22471cf9fc088d48b59155b42044767a1e/agent/src/main/java/kanela/agent/util/log/Logger.java#L43-L47

I guess the solution is to manually control the tinylog writing thread as documented here:

https://tinylog.org/v1/configuration#writingthread

nadavwr commented 2 years ago

This leaves us with a non-daemon thread that prevents graceful JVM exit. How do we retain the previous behavior?

jtjeferreira commented 2 years ago

This leaves us with a non-daemon thread that prevents graceful JVM exit. How do we retain the previous behavior?

Did you try this from https://tinylog.org/v1/configuration#writingthread:

For this purpose, the static method Configurator.shutdownWritingThread() can be called

dpsoft commented 2 years ago

@nadavwr @jtjeferreira I've opened a new issue to correct this behavior and avoid the leak: https://github.com/kamon-io/kanela/issues/138

ivantopo commented 2 years ago

@dpsoft why don't we completely remove asynchronous logging from Kanela? There really isn't a log being logged in normal operations. In fact, in most applications the only thing we will ever log is the Kanela banner :smile:

I wouldn't be concerned at all if all the logging coming out of Kanela was synchronous and we forget about these issues.

dpsoft commented 2 years ago

indeed, in normal operation the log level is warning and we shouldn't log anything, only in case of an unexpected error. +1 to make sync the logger. @jtjeferreira @nadavwr thoughts?

jtjeferreira commented 2 years ago

indeed, in normal operation the log level is warning and we shouldn't log anything, only in case of and unexpected error. +1 to make sync the logger. @jtjeferreira @nadavwr thoughts?

👍

nadavwr commented 2 years ago

This sounds like the right call -- IMO reliability should override performance considerations for this logger.

nadavwr commented 2 years ago

This leaves us with a non-daemon thread that prevents graceful JVM exit. How do we retain the previous behavior?

Did you try this from https://tinylog.org/v1/configuration#writingthread:

For this purpose, the static method Configurator.shutdownWritingThread() can be called

We'll stay at Kanela 1.0.13 for now in anticipation of sync logging, but this sounds like a good workaround for whomever comes across the issue. Thanks!

dvgica commented 2 years ago

FWIW the dangling non-daemon thread preventing JVM shutdown hit us, and I've spent an hour tracking this down. Look forward to seeing a resolution. The workaround of calling Configurator.shutdownWritingThread() from our application didn't seem to have the desired effect either, not sure why. Reverting to 1.0.13 did fix it though.