Remove usage of ThreadLocal for IHubs

[adamnegaard]

📜 Description

Migrate from using ThreadLocal to store IHub, since they are difficult to remove from framework managed threads.

💡 Motivation and Context

Fixes #2079.

💚 How did you test it?

Running the test suite.

📝 Checklist

• I reviewed the submitted code.
• I ran the test suite to verify the changes.
• No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
• I updated the docs if needed.
• Review from the native team if needed.
• No breaking change or entry added to the changelog.
• No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

[markushi]

Hey @adamnegaard, thank you for contributing to sentry-java!

difficult to remove from framework managed threads

Could you elaborate a bit more on this? Are you talking about e.g. thread pools which keep threads alive and thus don't close the hub?

[adinauer]

@markushi this refers e.g. to Tomcat where Threads are being reused but ThreadLocal variables keep holding on to Sentry hub instances. This leads to users having to restart their Tomcat instances once in a while.

The linked issue #2079 has some more details.

I've tried something similar to this but ran into problems (I have yet to find where I documented those).

@adamnegaard have you run this in production yet? How's that going, if so?

[getsantry]

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you remove the label Waiting for: Community, I will leave it alone ... forever!

---

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

[markushi]

@adinauer this PR looks pretty stale, we're closing it for now. Feel free to re-open if this gets relevant again.

[adinauer]

In case we come back to this PR to give it a try in the next major, we should also clear the map on close.

[adamnegaard]

Sorry for being so slow on this. I have not run this in production yet. I'm a bit sure about the best way to go about testing this. Do you have any tips for me as to how i could test it, to be enough to be included in a future release? We are still dealing with this issue in production, but are just managing with manual restarts of tomcat now and then.

[adinauer]

We're in the middle of rewriting some parts of the SDK for the upcoming 8.x release so the PR won't be mergable as is. It should be rather straight forward to adapt it to the new IScopesStorage interface.

Do you have any tips for me as to how i could test it

• Basically spin up the application before and after the change and compare what lands in Sentry.
  • Keep an eye out for data that's missing
  • Also check for data that doesn't belong to errors/transactions/... you're comparing as they might indicate leaks
• Spin up the server locally, hit it with lots of requests, run GC a couple times then capture a thread dump
  • You can compare the results of before and after the change
  • There shouldn't be any additional objects being held on to (Hub, Scope, Attachment, Breadcrumb, tags etc.)
• Check whether your tomcat warnings go away when using the updated version

As this affects all of the SDK there is a lot more testing required that's why we haven't yet given this PR the love it deserves.

In 8.x of the Java SDK we'll have to retest whether this issue even exists anymore since we're introducing lifecycle tokens that you can call close on when you're done so maybe that already takes care of cleaning up ThreadLocals. I haven't gotten around to testing this yet.