Skip to content
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

Closed
kfernald opened this issue Mar 11, 2021 · 3 comments
Closed

Not multi-threaded safe #32

kfernald opened this issue Mar 11, 2021 · 3 comments

Comments

@kfernald
Copy link

Using:

  • ClimateControl 0.2.0
  • Ruby 2.2.2

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:

multitask :task1 do
  ClimateControl.modify(FOO: 'one') do
    puts "TASK1: #{ENV['FOO']}"
    sleep(3)
  end
end

multitask :task2 do
  sleep(1)
  puts "TASK2-middle: #{ENV['FOO']}"
  sleep(2.5)
  puts "TASK2-after: #{ENV['FOO']}"
end

multitask test: %i[task1 task2]

The expected output if it were multi-threaded safe would be:

TASK1: one
TASK2-middle:
TASK2-after:

But the real output is:

TASK1: one
TASK2-middle: one
TASK2-after:

Is the intent of the GEM to be multi-threaded safe? If not I think it should be clear on the documentation page.

joshuaclayton added a commit that referenced this issue Mar 17, 2021
What?
=====

This demonstrates a thread safety issue highlighted in #32
@joshuaclayton
Copy link
Contributor

@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 ClimateControl.modify({}) block, things should work, but they won't run in parallel.

Why? Because the modification wraps ENV in a mutex. If there's contention (the env being used - including potentially mutating values), it blocks until the value is freed (we shift out of the Ruby block).

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.

@JoshCheek
Copy link
Contributor

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 want that test to work, you will have to monkey patch ENV itself.

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

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]

dorianmariecom pushed a commit that referenced this issue May 25, 2022
* 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)
dorianmariecom pushed a commit that referenced this issue May 26, 2022
* 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)
@dorianmariecom
Copy link

I've added documentation about thread safety, I don't think we want to replace the ENV and wrapping in a ClimateControl.modify { } block is a good solution in my humble opinion.

@dorianmariecom dorianmariecom closed this as not planned Won't fix, can't repro, duplicate, stale May 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants