Skip to content

Change strategy for detaching contexts#383

Merged
reyang merged 3 commits into
open-telemetry:masterfrom
pyohannes:fix-context-order
Oct 30, 2020
Merged

Change strategy for detaching contexts#383
reyang merged 3 commits into
open-telemetry:masterfrom
pyohannes:fix-context-order

Conversation

@pyohannes
Copy link
Copy Markdown
Contributor

@pyohannes pyohannes commented Oct 30, 2020

With the existing solution, users had to make sure that they always detached contexts in exactly the reverse orders they were attached. So, for attaching contexts in the order A - B - C, they have to be detached in the order C - B - A. If a user would try to detach B before C, this would lead to an error (which the user doesn't notice if they don't explicitly check the return code of Detach). This can lead problems if a user forgets to detach a context (in this case, this context would never be detached).

With this PR the strategy is changed so that if a context is detached, all it's active child contexts are detached too. So if a user attaches contexts in the order A - B - C, and then detached B, both B and C will be detached. This is a much more robust solution that will avoid dangling contexts.

Fixes #219.

@pyohannes pyohannes requested a review from a team October 30, 2020 03:47
@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 30, 2020

Codecov Report

Merging #383 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #383      +/-   ##
==========================================
+ Coverage   94.96%   94.98%   +0.02%     
==========================================
  Files         162      162              
  Lines        6968     6996      +28     
==========================================
+ Hits         6617     6645      +28     
  Misses        351      351              
Impacted Files Coverage Δ
...pi/include/opentelemetry/context/runtime_context.h 97.43% <100.00%> (+0.37%) ⬆️
api/test/context/runtime_context_test.cc 100.00% <100.00%> (ø)

bool Detach(Token &token) noexcept override
{
if (!(token == GetStack().Top()))
if (!GetStack().Contains(token))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess in 99% of the chance GetStack().Top() would equal to the token, so it might worth a special case optimization (might save few CPU cycles on L188?):

if (token == GetStack().Top())
{
  GetStack().Pop();
  return true;
}

// check if token exists on the stack and pop items above it

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Added.

Copy link
Copy Markdown
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@pyohannes pyohannes added the pr:please-merge This PR is ready to be merged by a Maintainer (rebased, CI passed, has enough valid approvals, etc.) label Oct 30, 2020
@reyang reyang merged commit be1bbad into open-telemetry:master Oct 30, 2020
GerHobbelt pushed a commit to GerHobbelt/opentelemetry-cpp that referenced this pull request Aug 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr:please-merge This PR is ready to be merged by a Maintainer (rebased, CI passed, has enough valid approvals, etc.)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Memory management and asynchronous span handling for Context API

2 participants