dry-effects
dry-effects copied to clipboard
dry-rb
Thread variables are not accessible
Describe the bug
Using state effect resets all Thread.current variables.
To Reproduce
include Dry::Effects::Handler.State(:updated_attributes)
include Dry::Effects.State(:updated_attributes)
def test
puts "outside: #{Thread.current.keys}"
clients_changes, result = with_updated_attributes(Hash.new { |hash, key| hash[key] = [] }) do
puts "inside: #{Thread.current.keys}"
end
end
Thread.current[:foo] = 'bar'
test
Oututs:
outside: [:foo, :dry_effects_stack]
inside: [:dry_effects_stack]
Expected behavior
As you can see variables set outside of the with_updated_attributes block are not visible inside the block.
I expect the following output:
outside: [:foo, :dry_effects_stack]
inside: [:foo, :dry_effects_stack]
My environment
- Affects my production application: NO
- Ruby version: "2.6.5"
- OS: MacOS
- Dry Effects version: 0.1.5
hello @bolshakov! looks like this is not a bug, this is how ruby fiber-local variables works:
f = Fiber.new do
puts "Fiber started"
Thread.current[:in_fiber_context] = true
Fiber.yield
puts "Fiber-local value: #{Thread.current[:in_fiber_context].inspect}"
puts "Fiber finished"
end
puts "Starting value: #{Thread.current[:in_fiber_context].inspect}"
f.resume
puts "Value outside of fiber: #{Thread.current[:in_fiber_context].inspect}"
f.resume
puts "Ending value: #{Thread.current[:in_fiber_context].inspect}"
# >> Starting value: nil
# >> Fiber started
# >> Value outside of fiber: nil
# >> Fiber-local value: true
# >> Fiber finished
# >> Ending value: nil
sample copied from this post
@flash-gordon correct me if i'm wrong
Yeah, absolutely. It works "as expected" because ruby works this way. Fortunately, all thread local usages can be replaced by the state effect. It should be mentioned in the docs so I'll leave it open until then.
I considered this library a great abstraction tool for dependency injection and algebraic effects. The knowledge about underlying fiber use is hidden and never mentioned.
Fortunately, all thread local usages can be replaced by the state effect.
It makes the library incompatible with other tools. Imagine a third-party library (say transaction manager) that uses thread locals. I'm not sure it's a good idea to force every library using thread-locals to migrate to dry-effects v0.1 to introduce it in the existing project.
I understand this argument may sound reasonable for new projects, but it makes hardly possible to introduce "dry-effects" into old ones. Rewriting a lot of code to use "dry-effects" may be a huge commitment.
Since the main purpose of this library is not the work with threads or fibers, I kindly suggest treating this as a bug and make it possible to cooperate "dry-effects" with threads-locals.
I considered this. The plan was to add a compatibility layer to the gem via extensions. For example, there's one for rspec. Making it work in general is though possible but performance-wise is suboptimal. Instead, one can add a list of variable names to be preserved in child fibers (we can have an API for that).
On the other hand, adding patches for common gems to dry-effects shouldn't be a problem since IMO libraries must encapsulate access to thread local variables, just as rspec does. Otherwise, it's a mess.
So far, I only needed to patch rspec which makes me think the problem doesn't happen too often. Do you have concrete examples where you would need it? For the record, I do use transactions from Sequel but I've never had issues with it even without patches. Even rspec breaks only when very specific features are used (i.e. pending).
Nice, I didn't know about extensions 👍 I implemented a patch myself but ended up using old good thread locals because they appeared to be more readable, and my code that uses "dry-effect" could lead to hard-to-debug issues in the future.
Do you have concrete examples where you would need it?
It was chewy, and something else that I couldn't remember now.
I agree this should be properly explained in the docs. Even if it's an edge-case it deserves an explanation.
Hey, to put some context
Thread[]= is for setting "fiber-local variable" https://ruby-doc.org/core-2.0.0/Thread.html#method-i-5B-5D
Thread#thread_variable_set can be used to set "thread-local variable" https://ruby-doc.org/core-2.0.0/Thread.html#method-i-thread_variable_set
The issue itself is rather hard to fix in one proper way, while it seems that Thread.current[...] = ... is often used by libraries to keep the state (initialized scope, context, agent, request store, etc).
Some related links:
- Sentry changed their implementation to use
thread_variable_*- https://github.com/getsentry/sentry-ruby/pull/1380/files#diff-22b275750d1e90117f2c54a2b886b774f2eee7a8a96b2c3cd029b4ca182a12d5R71 (problem will come with fiber-based web servers when transactions would be initialized inside fiber) - graphql-ruby added code to copy fiber-local variables between spawned fibers - https://github.com/rmosolgo/graphql-ruby/pull/3461/files#diff-b12d18ca9ae9a53e96af0cc2605c9615dd251940eef78e48cf6f787b77c98445R229 (also won't work with fiber-based web servers)
- dd-trace-ruby - there is open issue for that https://github.com/DataDog/dd-trace-rb/issues/1389
When doing some research I had an issue with https://github.com/newrelic/newrelic-ruby-agent/blob/dev/lib/new_relic/agent/instrumentation/sidekiq.rb - when having dry-effect based code it hide tracing information initialized by middleware.
@bolshakov @kml I spoke with @flash-gordon and agreed on a simple chewy refactoring that does doesn't imply the adoption of dry-effects there.
After that, we (dry-rb team) will provide an extension for chewy from dry-effects.
