Open bolshakov opened 3 years ago
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.
Definitely something to improve :)
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:
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)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.
Is this a problem only in case of the State effect?
@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
.
Describe the bug
Using state effect resets all
Thread.current
variables.To Reproduce
Oututs:
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:My environment