-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix(node): Convert debugging code to callbacks to fix memory leak in LocalVariables
integration
#7637
Conversation
LocalVariables
integration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! We can think about using await import
later, for now it's more important to fix/unblock this IMHO 👍
popped(result); | ||
} catch (_) { | ||
// If there is an error, we still want to call the complete callback | ||
complete(result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we clear the callbacks list here? Otherwise there is a risk of complete
being called multiple times right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it can only get called twice if a function passed to add
calls next
before throwing and in every current usage next
is called last.
Maybe add a throw if complete
is called and there are still callbacks remaining?
} | ||
|
||
function next(result: T): void { | ||
const popped = callbacks.pop() || complete; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we no-op if callbacks is empty? Perhaps give some kind of indicator of success (boolean return) from the next
function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we add complete
to the top of the callbacks stack, the only way next
can be called when callbacks is empty is if you call next
more than once per add
ed closure or manage to call next
in complete
(which would be tricky to do since you have to supply complete
before next
is returned). The || callback
here is mainly to keep the types happy and does not get hit currently.
If we don't expect to hit this path, maybe it would be better to throw if pop
returns undefined?
Closes #7230
This PR converts the
LocalVariables
debugger code to callbacks so that we can callDebugger.resume
synchronously from thepause
callback which fixes the memory leak.It's worth noting that now there is no usage of async, it will not be possible to use async import to improve this:
sentry-javascript/packages/node/src/integrations/localvariables.ts
Lines 32 to 35 in 0743e98
I created a basic Express server to test these changes and load tested an endpoint with a caught/handled error to see if any memory is leaked. Without the async code, performance is also much improved: