-
-
Notifications
You must be signed in to change notification settings - Fork 432
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
Remove usage of ThreadLocal for IHubs #2711
Conversation
Hey @adamnegaard, thank you for contributing to
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? |
@markushi this refers e.g. to Tomcat where Threads are being reused but 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? |
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 "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
@adinauer this PR looks pretty stale, we're closing it for now. Feel free to re-open if this gets relevant again. |
hub.close(); | ||
// remove the current threads hub from the map | ||
hubMap.remove(currentThread.getId()); | ||
} |
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.
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
.
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.
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.
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.
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.
📜 Description
Migrate from using
ThreadLocal
to storeIHub
, 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
sendDefaultPII
is enabled.🔮 Next steps