Document lack of thread safety

[balexand]

It is important to document the thread-safety limitations of this gem. Doing so could avoid serious bugs in other gems like thoughtbot/paperclip#1709. Thanks!

[balexand]

This paragraph refers to the changes made in #9 (refs #6).

[jferris]

With the addition of a mutex, Climate Control is thread safe, but not reentrant. The mutex means only one thread will enter the code at once. This may affect performance, but will not cause bugs. I don't believe it should be avoided in a multi-threaded environment.

[balexand]

@jferris You are correct that only one thread will enter the Climate Control code at once. But since Climate Control doesn't own the global ENV, it is possible that other threads will read to or write from ENV concurrently.

Hopefully these examples will illustrate the problems:

Example 1

This example shows that even if Climate Control were thread-safe, it's modification of global state could cause conflicts in other threads in an application.

require 'climate_control'

Thread.new do
  ClimateControl.modify('PATH' => '/custom/path/for/block') do
    sleep 1
  end
end

sleep 0.5

puts ENV['PATH'] # => /custom/path/for/block
puts `echo hello world` # raises No such file or directory - echo (Errno::ENOENT)

At the very least, the README should contain a warning like:

Internally, this gem modifies then reverts the global ENV variable. During the call to ClimateControl.modify, all other threads will see the modified ENV. This may cause conflicts in a multi-threaded environment.

Example 2

This example produces a race condition that causes an environment variable to be deleted. This shows that Climate Control is not thread-safe. Race conditions are notoriously difficult to reproduce, so I monkey patched in a sleep statement that ensures that the race condition is hit every time.

require 'climate_control'

# Add a delay to the method in order to consistently reproduce the race condition
ClimateControl::Modifier::OverlappingKeysWithChangedValues.class_eval do
  def keys_with_delay
    keys_without_delay.tap { sleep 1 }
  end

  alias_method :keys_without_delay, :keys
  alias_method :keys, :keys_with_delay
end


Thread.new do
  ClimateControl.modify('FOO' => 'temporary') do
  end
end

sleep 0.5

ENV['FOO'] = 'b'

puts ENV['FOO'].inspect  # => "b"

sleep 1

puts ENV['FOO'].inspect # => nil

[balexand]

Also, the lazy initialization of env on this line is not thread-safe. If multiple threads were to call ClimateControl.env concurrently, then it would be possible to initialize multiple instances of ClimateControl::Environment. In this case, you would have multiple mutex instances.

[jferris]

@balexand thanks for the detailed response.

But since Climate Control doesn't own the global ENV, it is possible that other threads will read to or write from ENV concurrently.

That's true; since we don't own ENV ourselves, we can't promise that other threads won't modify or use it. I think it would be good to have a README update warning users not to use or update ENV in other threads. This isn't really specific to Climate Control, though.

Also, the lazy initialization of env on this line is not thread-safe.

Good catch. Could you open an issue for that one? Code Climate should at least be thread-safe when using its public API, so I think that's a bug.

[balexand]

I'll make one more argument for why this gem should be documented as being not thread-safe. Then I'll leave it up to the maintainers of the gem to decide how to word the thread-safety warning.

@jferris wrote in the previous comment:

[Climate Control] should at least be thread-safe when using its public API

If ENV were considered to be part of Climate Control's private API, then Climate Control would be thread-safe. But ENV is not part of Climate Control's private API. It's part of the Ruby platform's public API (docs).

This is what I'm describing in the README when I write that this gem is

thread-safe if and only if no thread in your app reads to or writes from ENV while ClimateControl.modify is executing

[geoffharcourt]

I think this has been addressed now as of #24? LMK and we can re-open if it's still a concern.

[nickcharlton]

I think so yeah. I'm going to close this and we can re-open it if someone finds that not to be the case.