fix(node): Convert debugging code to callbacks to fix memory leak in LocalVariables integration#7637
Conversation
LocalVariables integration
mydea
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
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 added 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
LocalVariablesdebugger code to callbacks so that we can callDebugger.resumesynchronously from thepausecallback 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: