-
Notifications
You must be signed in to change notification settings - Fork 438
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
Cleanup of context from Runtime storage #885
Conversation
Codecov Report
@@ Coverage Diff @@
## main #885 +/- ##
==========================================
+ Coverage 95.40% 95.40% +0.01%
==========================================
Files 158 158
Lines 6705 6717 +12
==========================================
+ Hits 6396 6408 +12
Misses 309 309
|
// the shared_ptr object (if stored in prev context object ) are released. | ||
// The stack is not resized, and the unused memory would be reutilised | ||
// for subsequent context storage. | ||
base_[size_ - 1] = Context(); |
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.
How could someone get a shared_ptr
of the Context
stored in ThreadLocalContextStorage
?
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.
Line 237 is actually resetting the base[size - 1]
with empty Context() object. This is to ensure that previously-stored shared_ptr<Span>
gets released. The new empty Context() is not supposed to be used again.
@maxgolov - Would like to include these changes in the next release. Would it be good to merge, as you wanted to do some multi-threaded tests with these changes? |
@maxgolov Merging this. If there are issues found in your testing, we can address them separately. Thanks. |
Fixes #884
Changes
ThreadLocalContextStorage
is used as default runtime storage, and is internally implemented as aStack
. The Pop() operation doesn't perform the cleanup of storedContext
object, but only decrements the stack size. The actual cleanup happens only when that storage is later re-utilized with a new Context. The side effect of this approach is that anyshared_ptr
object stored in the popped-out Context will not be cleaned up at the time of Pop() operation.The fix is to replace the current context (meant to be popped out) with an empty
Context
object before decrementing the size.This will release the ownership of any
shared_ptr
object stored in this current context.Also, Stack::Pop() shouldn't be returning the removed object, to be consistent with std::stack api.
For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes