Thread-safety?
timhabermaas opened this issue · 2 comments
Affect.capture
doesn't seem thread-safe because it mutates the current context (@@current
) class variable and Affect
is shared across threads.
For example:
t1 = Thread.new {
Affect.capture(log: -> (message) { puts "logging 1: #{message}" }) do
Affect.perform(:log, "foo")
end
}
t2 = Thread.new {
Affect.capture(log: -> (message) { puts "logging 2: #{message}" }) do
Affect.perform(:log, "bar")
end
}
t1.join
t2.join
might end up printing "logging 2: foo\nlogging 2: bar\n"
instead of "logging 1: foo\nlogging 2: bar\n"
.
This specific issue can be fixed by using Thread.current[:current_ctx]
to attach the current context to the current thread (do you want me to open a PR?). However: I think this will lead to other issues when using Thread
s in user-land [1].
So I looked into using the fiber implementation which doesn't mutate global state, so the above code works fine. I believe it has the same problem when using Thread.new
around perform
as the Thread.current
solution, though. Fibers don't seem to play nicely with threads.
Do you have any experience using your affect
library in a multi-threaded environment? Maybe it's best to simply ban threads from user-land?
[1] The part of a code base which only cares about perform
, not about how to handle the effects.
For my own use, up until now I have used a single global context, so there was no problem with multi-threaded code.
For the moment, the behaviour of concurrent capture blocks is undefined. Using a thread-local variable is a solution, but I think a more comprehensive solution would be to allow threads to "inherit" the context of the thread doing the call to Thread.new
. This would necessitate a patch to Thread.new
. Also note that in Ruby thread-local variables are actually fiber-local, so this might affect the behaviour when running multiple fibers.
If you have a suggestion for how to solve this I'd like to know. Thanks for your interest!
Closing due to inactivity.