Hubs/Scopes Merge 15 - Replace ThreadLocal with scope storage#3317
Hubs/Scopes Merge 15 - Replace ThreadLocal with scope storage#3317
ThreadLocal with scope storage#3317Conversation
…pes-merge-2-add-scopes
adinauer
changed the title
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.
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.
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.
3 participants
#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