Scope the shoryuken context to the current fiber. #765
Conversation
|
This PR is now marked as stale because it hasn't seen activity for a while. Add a comment or it will be closed soon. |
|
@phstc do you have time to review? Thanks! |
|
Hi @ioquatix Sorry, I'm a bit out of the loop here. Are there any breaking changes here?
Asking in case this is good to go as a patch version or if there is any concern with a specific version of Ruby or Rails. |
|
In general, I don't believe this is a breaking change. |
|
This PR is now marked as stale because it hasn't seen activity for a while. Add a comment or it will be closed soon. |
|
This PR was closed because it hasn't seen activity for a while. |
|
@phstc any update? |
|
@ioquatix, Unfortunately, I'm not currently active with Ruby or Shoryuken. I'm mainly merging maintenance pull requests. It would be nice to have input from an active user. Can you try the Slack channel? https://join.slack.com/t/shoryuken/shared_invite/zt-19xjq3iqc-KmoJ6eU6~qvZNqcLzIrjww |
|
This PR is now marked as stale because it hasn't seen activity for a while. Add a comment or it will be closed soon. |
|
This PR was closed because it hasn't seen activity for a while. |
WalkthroughReplaced thread-local logging context with fiber-local storage by extending Fiber with a shoryuken_context accessor. Updated context retrieval and with_context handling to use Fiber.current. Context may now return nil when unset. No other logging formatting logic changed. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant LoggingBase as Shoryuken::Logging::Base
participant Fiber as Fiber.current
participant Logger
Caller->>LoggingBase: with_context(msg) { ... }
activate LoggingBase
LoggingBase->>Fiber: shoryuken_context = msg
Caller->>LoggingBase: context
LoggingBase->>Fiber: read shoryuken_context
Fiber-->>LoggingBase: msg or nil
LoggingBase->>Logger: log(message, context)
LoggingBase->>Fiber: shoryuken_context = nil (ensure)
deactivate LoggingBase
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
lib/shoryuken/logging.rb(3 hunks)
🧰 Additional context used
🪛 RuboCop (1.76.1)
lib/shoryuken/logging.rb
[convention] 18-18: Space missing to the left of {.
(Layout/SpaceBeforeBlockBraces)
[convention] 18-18: Space between { and | missing.
(Layout/SpaceInsideBlockBraces)
[convention] 18-18: Space missing inside }.
(Layout/SpaceInsideBlockBraces)
| class Fiber | ||
| attr_accessor :shoryuken_context | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid globally monkey-patching Fiber; prefer a scoped storage approach
Adding a public attr_accessor to Fiber introduces a global method that may clash with other libraries and is hard to reason about. You can avoid this by storing the context as an instance variable on the current fiber without adding methods to Fiber.
Apply this diff to remove the monkey patch:
-class Fiber
- attr_accessor :shoryuken_context
-endFollow-up diffs for lines 18, 36, and 39 update the access to use instance_variable_get/set (see separate comments).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class Fiber | |
| attr_accessor :shoryuken_context | |
| end |
🤖 Prompt for AI Agents
In lib/shoryuken/logging.rb around lines 6 to 8, remove the global Fiber
monkey-patch (the attr_accessor :shoryuken_context) and instead store context
directly on the current fiber as an instance variable; replace any reads/writes
to Fiber#shoryuken_context with
Fiber.current.instance_variable_get(:@shoryuken_context) and
Fiber.current.instance_variable_set(:@shoryuken_context, value). Remove the
class Fiber ... end attr_accessor block and ensure follow-up changes on lines
18, 36, and 39 use instance_variable_get/set as noted.
| Fiber.current.shoryuken_context&.then{|context| " #{context}"} | ||
| end |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Behavior change: thread-local to fiber-local is likely breaking in fiber-heavy apps
Previously, context was thread-local, so all fibers in the same thread shared it. After this change, context is fiber-local and won’t automatically propagate to new fibers, which may surprise users (e.g., Async/Fiber scheduler, fan-out patterns). The PR mentions this, but the code currently provides no migration path or configuration.
- Confirm that Shoryuken’s supported environments won’t be affected by losing context across fibers.
- If backward compatibility is required, consider introducing a configurable strategy, defaulting to thread-local for existing users.
Option A: Provide a configurable strategy (default: :thread, opt-in :fiber):
# e.g., Shoryuken::Logging.context_scope = :thread or :fiber
def self.current_context_get
case Shoryuken::Logging.context_scope
when :fiber
Fiber.current.instance_variable_get(:@shoryuken_context)
else
Thread.current[:shoryuken_context]
end
end
def self.current_context_set(value)
case Shoryuken::Logging.context_scope
when :fiber
Fiber.current.instance_variable_set(:@shoryuken_context, value)
else
Thread.current[:shoryuken_context] = value
end
endOption B (Ruby 3.1+): Use Fiber-local storage (Fiber.current[:shoryuken_context]) for clarity and potential inheritance if you implement it, gated by Ruby version check; fallback to thread-local otherwise.
To assess blast radius, search for any code relying on thread-local context:
Also applies to: 36-39
🏁 Script executed:
#!/bin/bash
# Find any remaining uses of the old thread-local context or behavior assumptions.
rg -n "Thread\.current\[:shoryuken_context\]|\bwith_context\b"Length of output: 419
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== lib/shoryuken/logging.rb ==="
nl -ba lib/shoryuken/logging.rb | sed -n '1,240p'
echo
echo "=== lib/shoryuken/processor.rb ==="
nl -ba lib/shoryuken/processor.rb | sed -n '1,240p'
echo
echo "=== spec/shoryuken/processor_spec.rb ==="
nl -ba spec/shoryuken/processor_spec.rb | sed -n '1,240p'Length of output: 237
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== lib/shoryuken/logging.rb ==="
if [ -f lib/shoryuken/logging.rb ]; then
sed -n '1,240p' lib/shoryuken/logging.rb
else
echo "MISSING: lib/shoryuken/logging.rb"
fi
echo
echo "=== lib/shoryuken/processor.rb ==="
if [ -f lib/shoryuken/processor.rb ]; then
sed -n '1,240p' lib/shoryuken/processor.rb
else
echo "MISSING: lib/shoryuken/processor.rb"
fi
echo
echo "=== spec/shoryuken/processor_spec.rb ==="
if [ -f spec/shoryuken/processor_spec.rb ]; then
sed -n '1,240p' spec/shoryuken/processor_spec.rb
else
echo "MISSING: spec/shoryuken/processor_spec.rb"
fi
echo
echo "=== Search for context-related usages ==="
rg -n "Thread\\.current\\[:shoryuken_context\\]|Fiber\\.current\\.shoryuken_context|shoryuken_context|with_context|context_scope" || true
echo
echo "=== Check gemspecs / ruby version hints ==="
rg -n "required_ruby_version|required_ruby_version\\s*=|s.required_ruby_version|.ruby-version|ruby:|matrix:.*ruby" || true
echo
if [ -f .ruby-version ]; then
echo ".ruby-version:"
cat .ruby-version
else
echo "no .ruby-version file"
fi
echo
echo "=== GitHub workflows (ruby matrix) ==="
if [ -d .github/workflows ]; then
for wf in .github/workflows/*.yml .github/workflows/*.yaml; do
[ -f "$wf" ] || continue
echo "---- $wf ----"
sed -n '1,240p' "$wf"
echo
done
else
echo "no .github/workflows directory"
fiLength of output: 14642
Breaking change: logging context moved from thread-local → fiber-local — needs compatibility option or docs
Short summary: I verified the change — logging now adds Fiber#shoryuken_context and uses Fiber.current for context/get/set (see lib/shoryuken/logging.rb). The repo requires Ruby >= 3.1 and CI runs 3.1–3.4, so Fiber-based storage is supported, but this does change semantics (contexts no longer automatically propagate across fibers). There are no other references to Thread.current[:shoryuken_context] in the repo.
Files to inspect / attention points
- lib/shoryuken/logging.rb
- attr_accessor :shoryuken_context (adds fiber-local slot)
- context:
Fiber.current.shoryuken_context&.then{|context| " #{context}"}(fiber-local read) - with_context: sets/resets
Fiber.current.shoryuken_context(fiber-local write)
- lib/shoryuken/processor.rb — uses Shoryuken::Logging.with_context to set context
- spec/shoryuken/processor_spec.rb — expects with_context to be called (no tests for propagation across fibers)
Recommended action (fix required)
- Provide a configurable compatibility strategy (default to :thread for existing users; allow :fiber opt-in). Example implementation to add to Shoryuken::Logging:
# default to :thread for backwards compatibility
@context_scope = :thread
def self.context_scope
@context_scope
end
def self.context_scope=(val)
@context_scope = val
end
def self.current_context_get
case context_scope
when :fiber
Fiber.current.shoryuken_context
else
Thread.current[:shoryuken_context]
end
end
def self.current_context_set(value)
case context_scope
when :fiber
Fiber.current.shoryuken_context = value
else
Thread.current[:shoryuken_context] = value
end
end- Use these helpers in Base#context and in with_context (set via current_context_set and clear via current_context_set(nil)).
- Add a changelog entry and/or migration note calling out the behavior change if you decide to make :fiber the default.
- Add tests that verify behavior when using fibers (propagation / isolation) if fiber-local becomes the default.
Rationale: the change is limited to logging.rb (no other Thread.current uses remain), but it is a behavioral change that can surprise fiber-heavy apps. If maintaining backward compatibility matters, implement the configurable strategy above (or document the breaking change clearly).
🧰 Tools
🪛 RuboCop (1.76.1)
[convention] 18-18: Space missing to the left of {.
(Layout/SpaceBeforeBlockBraces)
[convention] 18-18: Space between { and | missing.
(Layout/SpaceInsideBlockBraces)
[convention] 18-18: Space missing inside }.
(Layout/SpaceInsideBlockBraces)
🤖 Prompt for AI Agents
In lib/shoryuken/logging.rb around lines 18-19, the logging context was switched
from Thread-local to Fiber-local which changes propagation semantics; add a
configurable compatibility strategy (default to :thread) so existing users keep
thread-local behavior unless they opt into :fiber. Implement module-level
accessors context_scope and context_scope=, helper methods current_context_get
and current_context_set that read/write either
Thread.current[:shoryuken_context] or Fiber.current.shoryuken_context based on
context_scope, and replace direct Fiber.current reads/writes in Base#context and
with_context with these helpers (clearing via current_context_set(nil)). Also
add tests covering both :thread and :fiber modes and a changelog/migration note
if you change the default in the future.
💡 Verification agent
🧩 Analysis chain
Fix style offenses and avoid Kernel#then for broader Ruby compatibility
The safe-navigation + then pattern triggers RuboCop spacing offenses and depends on Kernel#then (Ruby 2.6+). A simpler expression avoids both issues and keeps behavior identical.
Apply this diff:
- Fiber.current.shoryuken_context&.then{|context| " #{context}"}
+ if (context = Fiber.current.instance_variable_get(:@shoryuken_context))
+ " #{context}"
+ endNotes:
- This removes the Style/Layout offenses flagged by RuboCop (spacing around braces).
- It also avoids relying on Kernel#then, keeping compatibility with older Rubies if Shoryuken still supports them.
If you prefer to keep using Kernel#then, please confirm the minimum supported Ruby version in the gemspec:
Or verify via web search when Kernel#then was introduced:
🏁 Script executed:
#!/bin/bash
# Check the minimum supported Ruby version to assess if Kernel#then is acceptable.
fd -a -t f -HI '\.gemspec$' | while read -r f; do
echo "== $f"
rg -n 'required_ruby_version|spec.required_ruby_version' "$f" || true
doneLength of output: 283
🌐 Web query:
When was Kernel#then introduced in Ruby, and what versions support it?
💡 Result:
Short answer
- Kernel#then (an alias for Kernel#yield_self) was added in MRI Ruby 2.6.0. (ruby-lang.org, bugs.ruby-lang.org)
Details & dates
- yield_self was added in Ruby 2.5 (Ruby 2.5.0 released Dec 25, 2017). (ruby-lang.org)
- then was introduced as an alias for yield_self and shipped in Ruby 2.6.0 (released Dec 25, 2018). (ruby-lang.org, ruby-doc.org)
What versions support it
- Any standard MRI (CRuby) 2.6.0 and later support Kernel#then (so 2.6.x, 2.7.x, 3.x, etc.). The method is simply an alias of yield_self added in the 2.6 changes. (ruby-doc.org, ruby-lang.org)
If you need
- I can check a specific Ruby implementation (JRuby, TruffleRuby, mruby) or show how to detect availability in code (e.g., respond_to?(:then)).
Citations:
- 1: https://www.ruby-lang.org/en/news/2018/12/25/ruby-2-6-0-released//?utm_source=chatgpt.com
- 2: https://bugs.ruby-lang.org/issues/14594?utm_source=chatgpt.com
- 3: https://www.ruby-lang.org/en/news/2017/12/25/ruby-2-5-0-released/?utm_source=chatgpt.com
- 4: https://www.ruby-lang.org/en/news/2018/12/25/ruby-2-6-0-released//?utm_source=chatgpt.com
- 5: https://ruby-doc.org/2.7.7/NEWS-2_6_0.html?utm_source=chatgpt.com
- 6: https://ruby-doc.org/2.7.7/NEWS-2_6_0.html?utm_source=chatgpt.com
- 7: https://www.ruby-lang.org/en/news/2018/12/25/ruby-2-6-0-released//?utm_source=chatgpt.com
Keep Kernel#then (gemspec ≥ 3.1.0) — just fix the RuboCop spacing
Brief: the gemspec requires Ruby >= 3.1.0 (so Kernel#then, added in Ruby 2.6, is available). The remaining problem is a RuboCop spacing offense; please fix the block-brace spacing.
- File: lib/shoryuken/logging.rb — lines 18-19
Apply this diff:
- Fiber.current.shoryuken_context&.then{|context| " #{context}"}
+ Fiber.current.shoryuken_context&.then { |context| " #{context}" }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Fiber.current.shoryuken_context&.then{|context| " #{context}"} | |
| end | |
| Fiber.current.shoryuken_context&.then { |context| " #{context}" } | |
| end |
🧰 Tools
🪛 RuboCop (1.76.1)
[convention] 18-18: Space missing to the left of {.
(Layout/SpaceBeforeBlockBraces)
[convention] 18-18: Space between { and | missing.
(Layout/SpaceInsideBlockBraces)
[convention] 18-18: Space missing inside }.
(Layout/SpaceInsideBlockBraces)
🤖 Prompt for AI Agents
In lib/shoryuken/logging.rb around lines 18 to 19, adjust the block-brace
spacing to satisfy RuboCop: change the inline block from &.then{|context| "
#{context}"} to use a space before the brace and spaces inside the braces like
&.then { |context| " #{context}" } so the method and block spacing conforms to
style.
| def self.with_context(msg) | ||
| Thread.current[:shoryuken_context] = msg | ||
| Fiber.current.shoryuken_context = msg | ||
| yield | ||
| ensure | ||
| Thread.current[:shoryuken_context] = nil | ||
| Fiber.current.shoryuken_context = nil | ||
| end |
There was a problem hiding this comment.
Restore previous context on exit to support nested with_context blocks
Current ensure unconditionally sets the context to nil, which breaks nested with_context usage. Example:
with_context("outer") { with_context("inner") { ... } }
after the inner block, context should be "outer", not nil.
Apply this diff to save and restore the previous context:
- def self.with_context(msg)
- Fiber.current.shoryuken_context = msg
- yield
- ensure
- Fiber.current.shoryuken_context = nil
- end
+ def self.with_context(msg)
+ fiber = Fiber.current
+ previous = fiber.instance_variable_get(:@shoryuken_context)
+ fiber.instance_variable_set(:@shoryuken_context, msg)
+ yield
+ ensure
+ fiber.instance_variable_set(:@shoryuken_context, previous)
+ endI can also add unit tests that validate:
- context is included in log output,
- nested with_context restores the outer value,
- context is isolated per fiber.
Would you like me to draft these specs?
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def self.with_context(msg) | |
| Thread.current[:shoryuken_context] = msg | |
| Fiber.current.shoryuken_context = msg | |
| yield | |
| ensure | |
| Thread.current[:shoryuken_context] = nil | |
| Fiber.current.shoryuken_context = nil | |
| end | |
| def self.with_context(msg) | |
| fiber = Fiber.current | |
| previous = fiber.instance_variable_get(:@shoryuken_context) | |
| fiber.instance_variable_set(:@shoryuken_context, msg) | |
| yield | |
| ensure | |
| fiber.instance_variable_set(:@shoryuken_context, previous) | |
| end |
🤖 Prompt for AI Agents
In lib/shoryuken/logging.rb around lines 35 to 40, the ensure block always sets
Fiber.current.shoryuken_context = nil which breaks nested with_context usage;
change the method to save the current Fiber.current.shoryuken_context into a
local variable before assigning the new msg, yield, and in ensure restore that
saved value (rather than unconditionally nil) so nested calls properly reinstate
the outer context and context remains fiber-local.
|
Note to self and everyone: I opened it to revisit this after talking with @ioquatix and not to merge it per se |
3 participants
I'd like to start a discussion about how to more efficiently manage the context during logging.
This exposes a proper interface for getting the current context.
In addition, I'd like to understand, do you expect if someone creates child fibers or threads, that the context should be shared? e.g. map-reduce / fan-out style workloads. I'm assuming the answer should be yes, but I'd like to know your opinion. In that case, it's better to use
Fiber#storage.Summary by CodeRabbit
Bug Fixes
Refactor