Skip to content
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

fix: Replace Context stack on clear #1598

Merged
merged 2 commits into from
Feb 15, 2024
Merged

Conversation

fbogsany
Copy link
Contributor

As identified in open-telemetry/opentelemetry-ruby-contrib#772, Rails copies all thread-local variables in ActionController::Live. Our use of an array to back the Context stack results in two threads sharing the same stack. Context.clear only clears the array, but continues to share it (in fact, its effect is visible to both threads), so there is no way to cleanly separate the thread's Context stacks using the public API.

This PR replaces the stack.clear call with assignment of a new array to the thread-local. This allows a pattern like:

locals = Thread.current.keys.map { |key| [key, t1[key]] }
Thread.new do
  t2 = Thread.current
  locals.each { |k, v| t2[k] = v }
  # Context stack is shared between parent and t2 here.
  # This is racy and requires something like a semaphore to avoid concurrent mutation.
  current_context = OpenTelemetry::Context.current
  OpenTelemetry::Context.clear
  # Context stacks are now independent. Signal semaphore.
  OpenTelemetry::Context.attach(current_context)
  ...
end

@fbogsany fbogsany merged commit dd11d5b into main Feb 15, 2024
55 checks passed
@robertlaurin robertlaurin deleted the clear-context-with-new-array branch February 20, 2024 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants