Skip to content

Scope the shoryuken context to the current fiber. #765

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.

Sign up for GitHub

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

Open
ioquatix wants to merge 3 commits into
base: main
Choose a base branch
from
+7 −4
Open

Scope the shoryuken context to the current fiber. #765

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9ef35d3
Scope the shoryuken context to the current fiber.
ioquatix Mar 6, 2024
4f9cc9e
Merge branch 'main' into patch-2
ioquatix Aug 14, 2025
32f7050
Merge branch 'main' into patch-2
mensfeld Aug 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions lib/shoryuken/logging.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@
require 'time'
require 'logger'

class Fiber
attr_accessor :shoryuken_context
end
Comment on lines +6 to +8
Copy link

@coderabbitai coderabbitai bot Aug 14, 2025

Choose a reason for hiding this comment

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

🛠️ 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
-end

Follow-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.

Suggested change
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.


module Shoryuken
module Logging
class Base < ::Logger::Formatter
Expand All @@ -11,8 +15,7 @@ def tid
end

def context
c = Thread.current[:shoryuken_context]
c ? " #{c}" : ''
Fiber.current.shoryuken_context&.then{|context| " #{context}"}
end
Comment on lines +18 to 19
Copy link

@coderabbitai coderabbitai bot Aug 14, 2025

Choose a reason for hiding this comment

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

💡 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
end

Option 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"
fi

Length 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}"
+        end

Notes:

  • 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
done

Length of output: 283

🌐 Web query:

When was Kernel#then introduced in Ruby, and what versions support it?

💡 Result:

Short answer

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:

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.

Suggested change
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.

end

Expand All @@ -30,10 +33,10 @@ def call(severity, _time, _program_name, message)
end

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
Comment on lines 35 to 40
Copy link

@coderabbitai coderabbitai bot Aug 14, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

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)
+    end

I 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.

Suggested change
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.


def self.initialize_logger(log_target = STDOUT)
Expand Down