Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(node): Local variables handle error #13827

Merged
merged 4 commits into from
Oct 4, 2024

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Sep 29, 2024

Ref #13768

Runtime.releaseObject can throw. I've reworked it to simplify and so that errors are caught.

After a lot of testing, I've removed the memory leak test. I've tested this PR extensively on Node v20 + v22 and while memory usage does peak at ~350MB, it doesn't get above that after 15 minute of 1k errors per second. Memory usage varies from 150-300MB but I've concluded it's not actually leaking.

Copy link
Contributor

github-actions bot commented Sep 29, 2024

size-limit report 📦

Path Size % Change Change
@sentry/browser 22.63 KB - -
@sentry/browser - with treeshaking flags 21.42 KB - -
@sentry/browser (incl. Tracing) 34.86 KB - -
@sentry/browser (incl. Tracing, Replay) 71.36 KB - -
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 61.79 KB - -
@sentry/browser (incl. Tracing, Replay with Canvas) 75.72 KB - -
@sentry/browser (incl. Tracing, Replay, Feedback) 88.49 KB - -
@sentry/browser (incl. Tracing, Replay, Feedback, metrics) 90.36 KB - -
@sentry/browser (incl. metrics) 26.91 KB - -
@sentry/browser (incl. Feedback) 39.77 KB - -
@sentry/browser (incl. sendFeedback) 27.29 KB - -
@sentry/browser (incl. FeedbackAsync) 32.08 KB - -
@sentry/react 25.38 KB - -
@sentry/react (incl. Tracing) 37.84 KB - -
@sentry/vue 26.8 KB - -
@sentry/vue (incl. Tracing) 36.75 KB - -
@sentry/svelte 22.76 KB - -
CDN Bundle 23.94 KB - -
CDN Bundle (incl. Tracing) 36.63 KB - -
CDN Bundle (incl. Tracing, Replay) 71.13 KB - -
CDN Bundle (incl. Tracing, Replay, Feedback) 76.44 KB - -
CDN Bundle - uncompressed 70.14 KB - -
CDN Bundle (incl. Tracing) - uncompressed 108.6 KB - -
CDN Bundle (incl. Tracing, Replay) - uncompressed 220.49 KB - -
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 233.71 KB - -
@sentry/nextjs (client) 37.8 KB - -
@sentry/sveltekit (client) 35.43 KB - -
@sentry/node 125.09 KB -0.04% -40 B 🔽
@sentry/node - without tracing 93.54 KB -0.04% -30 B 🔽
@sentry/aws-serverless 103.24 KB -0.04% -39 B 🔽

View base workflow run

@Bruno-DaSilva
Copy link

hah. I was just testing the new implementation and ran into this exact problem, was about to open a gh issue.

@timfish is it worth implementing some kind of heartbeat on the worker so we can detect (and log) when it's dead?

@Bruno-DaSilva
Copy link

Bruno-DaSilva commented Sep 29, 2024

And btw, a minimal reproducible example would likely be to throw an exception but immediately throw it away (+ let it be GC'd?) without doing a Sentry.captureException(). Perhaps that could be added as a test.

I believe what's happening is exceptions that get caught and thrown away before the 1s releaseObject timer runs result in this lovely inspector error:
Screen Shot 2024-09-29 at 8 19 11 AM

@Bruno-DaSilva
Copy link

As a final FYI i tested a try/catch in the worker and it has fixed the worker dying in my testing.

@timfish
Copy link
Collaborator Author

timfish commented Sep 29, 2024

so we can detect (and log) when it's dead?

If you have logging enabled via debug: true we log if the worker has errors or exits.

@AbhiPrasad AbhiPrasad merged commit 0d42ae8 into develop Oct 4, 2024
126 checks passed
@AbhiPrasad AbhiPrasad deleted the timfish/fix/local-variables-crash branch October 4, 2024 19:30
@timfish
Copy link
Collaborator Author

timfish commented Oct 10, 2024

The fix for this has been released in v8.34.0!

@chiragjn
Copy link

I just tried it out, weirdly objects of a class end up null :/
image

@timfish
Copy link
Collaborator Author

timfish commented Oct 18, 2024

@chiragjn this looks unrelated to this issue so can you open a new issue with a reproduction?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants