-
-
Couldn't load subscription status.
- Fork 458
Hubs/Scopes Merge 15 - Replace ThreadLocal with scope storage
#3317
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
Conversation
…pes-merge-2-add-scopes
ThreadLocal with scope storageThreadLocal with scope storage
|
| mainScopes = NoOpScopes.getInstance(); | ||
| // remove thread local to avoid memory leak | ||
| currentScopes.remove(); | ||
| getScopesStorage().close(); |
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.
If not in globalHubMode, is it enough to clean the ThreadLocal in the calling thread?
What about scopes stored in other long-living threads, like worker threads or threads in ThreadPools that are reused, wouldn't the scopes in there live on? Would have been the same for Hub before the Hubs/Scopes merge.
If one would re-init Sentry, the ThreadLocal on the calling thread is cleaned, if, however there were long-living threads that had a reference to the initial Scopes instance, they would still use the initial version of Scopes when calling Sentry.captureX for example.
Not sure about the implications, just wanted to point out a potential issue.
Please correct me if I'm wrong :)
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.
Yeah, maybe we can give https://github.com/getsentry/sentry-java/pull/2711/files a try once we're done with most changes for the major.
#skip-changelog
📜 Description
Replace
ThreadLocalinSentrywith scopes storage.💡 Motivation and Context
Can use token to clean up forked
Scopes. Can configure store to use OpenTelemetryContextunder the hood (in a follow up PR).💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps