-
Notifications
You must be signed in to change notification settings - Fork 289
Use fiber-local storage for logging context (Ruby 3.2+) #920
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
Conversation
Change logging context from Thread-local to Fiber-local storage to properly support fiber-based/async environments (e.g., Async gem, Falcon web server). Key improvements over the original PR #765: - Uses Ruby 3.2+ native Fiber[] storage instead of monkey-patching Fiber - Falls back to Thread.current for older Ruby versions - Properly preserves outer context in nested with_context calls - Adds current_context and context_storage public methods This prevents context "leaking" between fibers running on the same thread, which was causing incorrect log messages in async environments. Ref #765
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRefactors logging context storage from thread-local to fiber-local storage on Ruby 3.2+ (with thread-local fallback), adding public accessors Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Pull request overview
This PR updates Shoryuken's logging context management to use fiber-local storage (Ruby 3.2+) instead of thread-local storage, addressing context leakage issues in async/fiber-based environments. The implementation maintains backward compatibility with older Ruby versions and fixes nested context handling.
Key changes:
- Replace
Thread.current[]withFiber[]for logging context storage on Ruby 3.2+ - Implement proper nested context preservation by saving/restoring previous values instead of setting to nil
- Add
current_contextandcontext_storagehelper methods for accessing context state
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/shoryuken/logging.rb | Implements fiber-local storage with fallback to thread-local, adds context helper methods, and fixes nested context handling |
| lib/shoryuken/logging/base.rb | Updates context retrieval to use new current_context method instead of direct thread-local access |
| spec/lib/shoryuken/logging_spec.rb | Updates tests to use new context storage API and adds comprehensive tests for nested context preservation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Since shoryuken requires Ruby >= 3.2.0, the fallback to Thread.current for older Ruby versions is dead code. Fiber[] and Fiber[]= are always available in supported Ruby versions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
spec/lib/shoryuken/logging_spec.rb (1)
27-29: Make context cleanup restore the previous value (not forcenil).
Right now theafterhooks can clobber a context that existed before the example ran (reduces spec isolation).- after do - Shoryuken::Logging.context_storage[:shoryuken_context] = nil - end + around do |ex| + storage = Shoryuken::Logging.context_storage + previous = storage[:shoryuken_context] + storage[:shoryuken_context] = nil + ex.run + ensure + storage[:shoryuken_context] = previous + endAlso applies to: 48-50, 74-76, 97-99, 158-160
CHANGELOG.md (1)
2-6: Tighten changelog entry: format API names + add PR reference + clarify fallback/min-Ruby.
Consider using backticks (e.g.,Fiber[]) and referencing PR #920; also clarify whether Thread fallback exists or if Ruby 3.2+ is required here (to avoid mixed messaging).-- Enhancement: Use fiber-local storage for logging context - - Replaces thread-local storage with Fiber[] for proper isolation in async environments +- Enhancement: Use fiber-local storage for logging context (#920) + - Replaces thread-local storage with `Fiber[]` for proper isolation in async environments - Ensures logging context doesn't leak between fibers in the same thread - - Leverages Ruby 3.2+ fiber-local storage API + - Leverages Ruby 3.2+ fiber-local storage API (with Thread fallback where applicable)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)lib/shoryuken/logging.rb(1 hunks)spec/lib/shoryuken/logging_spec.rb(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/shoryuken/logging.rb
🧰 Additional context used
🧬 Code graph analysis (1)
spec/lib/shoryuken/logging_spec.rb (2)
lib/shoryuken/logging.rb (3)
context_storage(38-40)with_context(19-25)current_context(30-32)lib/shoryuken/logging/base.rb (1)
context(20-23)
🪛 ast-grep (0.40.0)
spec/lib/shoryuken/logging_spec.rb
[warning] 81-81: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: formatter.call('DEBUG', Time.now, 'program', 'debug message')
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Integrations (3.4)
- GitHub Check: Integrations (3.3)
- GitHub Check: Integrations (3.2)
- GitHub Check: Integrations (3.3)
- GitHub Check: Integrations (3.4)
- GitHub Check: Integrations (3.2)
🔇 Additional comments (3)
spec/lib/shoryuken/logging_spec.rb (3)
131-155: Nested context coverage (including inner exception) looks solid.
The outer restoration assertions match the newensure-based behavior.
172-175: Avoid hard-codingFiberin the spec if Thread fallback is supported.
If the library truly falls back on Ruby < 3.2 (per PR description), this expectation should be version-gated or behavior-based.- expect(described_class.context_storage).to eq(Fiber) + if Fiber.respond_to?(:[]) + expect(described_class.context_storage).to eq(Fiber) + else + expect(described_class.context_storage).to eq(Thread.current) + endAlternatively, assert the storage API contract instead:
storage = described_class.context_storage expect(storage).to respond_to(:[]) expect(storage).to respond_to(:[]=)
81-83: Ignore static-analysis “hardcoded RSA passphrase” warning here (false positive).
This is a log message string, not a credential.
Summary
This PR is a revised implementation of #765 by @ioquatix, addressing the need for fiber-local logging context in async environments.
The Problem
In fiber-based environments (Async gem, Falcon web server, etc.), multiple fibers can run on the same thread. Thread-local storage (
Thread.current[:shoryuken_context]) causes context to "leak" between fibers, resulting in incorrect log messages.Changes
Fiber[]storage (Ruby 3.2+) instead ofThread.current[]for logging contextThread.currentfor older Ruby versions (maintains backward compatibility)nilin ensure, destroying outer context)Shoryuken::Logging.current_contextmethod for direct context accessShoryuken::Logging.context_storagemethod to get the appropriate storage backendKey Improvements over PR #765
Fiber[]andFiber[]=instead of addingattr_accessorto FiberCode Example
Test plan
current_contextandcontext_storagemethodsRef #765
cc @ioquatix - would love your feedback on this approach!
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.