-
-
Notifications
You must be signed in to change notification settings - Fork 25
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.
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
Not multi-threaded safe #32
Comments
What? ===== This demonstrates a thread safety issue highlighted in #32
@kfernald ah, nice catch! Yes, we'd already addressed some thread-safety issues when separate threads are each using ClimateControl, but you've highlighted something here that I've looked to represent in edbc287 (this fails locally for me). If you wrap the second task in a Why? Because the modification wraps I'd be happy to review a PR that documents this particular case, or alternatively, if you have ideas for how to address this, happy to talk through them. Somewhat separately, I'd be interested in the actual use-case for this outside of tests where this would come up. It seems like environment variables might not be the optimal way to manage settings like this, so additional context would be appreciated. |
If you want that test to work, you will have to monkey patch The requirement seems sketchy to me, this is inter-process state, it's beyond global, how would it change according to which thread is looking at it? IMO, it would be better to talk to a wrapper object, not directly to Eg, something like this: class ThreadlocalEnvVars
def initialize(vars)
@vars, @overrides = vars, {}
end
def modify(overrides)
(@overrides[thread_id] ||= []).push overrides.transform_keys &:to_s
begin
yield
ensure
@overrides[thread_id].pop
@overrides.delete thread_id if @overrides[thread_id].empty?
end
end
def [](key)
key = key.to_s
hash = @overrides[thread_id].to_a.reverse_each.find { |h| h.key? key }
(hash || @vars)[key]
end
private def thread_id
Thread.current.object_id
end
end
EnvVars = ThreadlocalEnvVars.new(ENV)
multitask :task1 do
EnvVars.modify(FOO: 'one') do
puts "TASK1: #{EnvVars['FOO']}"
sleep(3)
end
end
multitask :task2 do
sleep(1)
puts "TASK2-middle: #{EnvVars['FOO']}"
sleep(2.5)
puts "TASK2-after: #{EnvVars['FOO']}"
end
multitask test: %i[task1 task2] |
* Why was this change necessary? I wanted to check if concurrent access worked correctly and make sure it will still work if there is a refactor. * How does it address the problem? It adds a test for this scenario. * Are there any side effects? This was already documented in GitHub but not tested: #32 (comment)
* Why was this change necessary? I wanted to check if concurrent access worked correctly and make sure it will still work if there is a refactor. * How does it address the problem? It adds a test for this scenario. * Are there any side effects? This was already documented in GitHub but not tested: #32 (comment)
I've added documentation about thread safety, I don't think we want to replace the |
Using:
ClimateControl is not currently multi-threaded safe, and the documentation doesn't make this clear. Running ClimateControl within a Rake "multitask" causes all other parallel tasks to have the modified environment.
Here's a sample set of rake tasks that demonstrate the issue:
The expected output if it were multi-threaded safe would be:
But the real output is:
Is the intent of the GEM to be multi-threaded safe? If not I think it should be clear on the documentation page.
The text was updated successfully, but these errors were encountered: