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

UncaughtExceptionHandlerIntegration leaks memory #3266

Open
xenomote opened this issue Mar 14, 2024 · 9 comments · May be fixed by #3398
Open

UncaughtExceptionHandlerIntegration leaks memory #3266

xenomote opened this issue Mar 14, 2024 · 9 comments · May be fixed by #3398
Assignees
Labels
Platform: Java Type: Bug Something isn't working

Comments

@xenomote
Copy link

xenomote commented Mar 14, 2024

Integration

sentry

Java Version

17

Version

7.6.0

Steps to Reproduce

Cannot be very specific because sentry SDK is being used by a plugin within a framework, and within that environment we cannot reproduce the issue unless we are receiving production traffic

Expected Result

no memory leak

Actual Result

the UncaughtExceptionHandlerIntegration appears to occupy the majority of the heap, eventually resulting in memory exhaustion and a crash.

On auditing the code it appears that

final Thread.UncaughtExceptionHandler currentHandler =
threadAdapter.getDefaultUncaughtExceptionHandler();
if (currentHandler != null) {
this.options
.getLogger()
.log(
SentryLevel.DEBUG,
"default UncaughtExceptionHandler class='"
+ currentHandler.getClass().getName()
+ "'");
defaultExceptionHandler = currentHandler;
}
threadAdapter.setDefaultUncaughtExceptionHandler(this);

causes a stack to form, which prevents any of the exeption handlers from being GC'd and results in OOM. No idea why that register method is being called over and over again

image

we are planning to simply turn the UncaughtExceptionHandler off in the options to resolve this issue, but it's weird that the Sentry SDK apparently behaves this way by default

@romtsn
Copy link
Member

romtsn commented Mar 14, 2024

I might be reading it wrong, but it looks like defaultExceptionHandler is already of instance UncaughtExceptionHandlerIntegration, which means someone set it before us manually, or the sdk has been initialized multiple times without calling Sentry.close() in between.

@xenomote
Copy link
Author

Yes I agree with you Roman, that does seem to be the case

Sadly I can't really give any detail on how this would be the case because it's a library used by a plugin used by a framework, all of which I have no direct control over aside from config

From what I understand from debugging the whole stack, init should only be called once at framework startup when the plugin is initialised, and captureEvent is called for each event to be logged

I have no idea how or why the register method is being called multiple thousands of times, but my assumption would have been that init would be idempotent

@romtsn
Copy link
Member

romtsn commented Mar 15, 2024

hm, but do you see that register is called multiple thousands of times actually?

My explanation would be that we just get into a infinite recursive call, because we delegate to defaultExceptionHandler here

defaultExceptionHandler.uncaughtException(thread, thrown);

and since it's our own handler, we'll get into the loop.

One improvement from our side would be to just check if defaultExceptionHandler is not instance of UncaughtExceptionHandlerIntegration before delegating to it to avoid the loop, that should not hurt.

@romtsn
Copy link
Member

romtsn commented Mar 15, 2024

it's a library used by a plugin used by a framework,

also, just realised - we don't really support this case out of the box (using our sdk in a library), because there might be another library/plugin which uses sentry and then they will have a conflict (including sending events to a wrong dsn/project). I assume this is precisely what happens here actually.

@xenomote
Copy link
Author

xenomote commented Mar 15, 2024

hm, but do you see that register is called multiple thousands of times actually?

my evidence for this is that defaultExceptionHandler is only ever assigned once- in the register method. To be clear, I don't think it is the register method being called on the same instance of UncaughtExceptionHandlerIntegration, because the register boolean gate will prevent it. It seems more like new instances of the handler are being created and registered repeatedly, thus forming a stack

because there might be another library/plugin which uses sentry

I can categorically guarantee that is not the case for my particular situation, we only have this one plugin which uses the sentry SDK, and no other component in the entire system makes use or reference of the Sentry SDK in any way

As far as I can understand it, the framework we use only initialises one instance of the plugin, and that one plugin instance only calls Sentry.init() once at startup, so I don't see how register is being called repeatedly, unless it is somehow triggered by a background thread within the Sentry SDK, or hub.captureEvent somehow causes it per-call

As a side note- hypothetically speaking, if calling init more than once is unsupported and results in this kind of unpleasant side effect, I would expect some critical error log or runtime exception to explicitly crash the application so that it's extremely obvious when this kind of misconfiguration happens so this kind of problem will reveal itself in development, not production

If crashing doesn't seem like a palatable choice then I would at least expect init to be idempotent with a prominent warning about init being called more than once. As it stands, I don't even know if this is the problem, but if it is then there was no way I could have known about it until we had it in prod and it killed a mission critical system with OOM 😢

@romtsn
Copy link
Member

romtsn commented Mar 15, 2024

yeah the init call is supposed to be idempotent, and we do print a warning here

if (isEnabled()) {
options
.getLogger()
.log(
SentryLevel.WARNING,
"Sentry has been already initialized. Previous configuration will be overwritten.");
}

without testing it locally however, I cannot say if we've overlooked something to clean up between multiple init calls, was just posting my assumptions here. Maybe @adinauer can take a closer look, I'm out of bandwidth for this unfortunately

@xenomote
Copy link
Author

no worries. Thank you for your attention Roman

I apologise for the somewhat vague issue report, but I'm only able to tell you as much as I know myself 😅

the init call is supposed to be idempotent, and we do print a warning here

Not sure where that is expected to be logged (in sentry itself or in stderr/stdout), but I don't see any evidence of it anywhere. My assumption is that init is only ever called once, but as I say I can't exactly confirm it. The fact that I don't see the warning printed anywhere would support that assumption

@lbloder
Copy link
Collaborator

lbloder commented Apr 2, 2024

Hi @xenomote,
Here's a short update on this issue:
I was able to reproduce locally now. We are currently working on a fix for this issue.

Thank you for your patience, we'll keep you updated.

@xenomote
Copy link
Author

xenomote commented Apr 2, 2024

excellent, thanks for your attention

I am not personally blocked on this issue, because I wasn't using this functionality anyway- I just disabled in the init config. But for the sake of others it would still be positive to fix 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Platform: Java Type: Bug Something isn't working
Projects
Status: No status
Status: Needs Discussion
Development

Successfully merging a pull request may close this issue.

4 participants