Skip to content

Commit

Permalink
Make ClimateControl.modify re-entrant
Browse files Browse the repository at this point in the history
With the lock added in 0.4, it's no longer safe to nest calls
`ClimateControl.modify`, which is actually a useful feature when e.g.
using `ClimateControl` in RSpec `around { ... }` blocks. Doing so
results in a deadlock (stack trace from the spec included in this
commit):

```
1) Climate control is re-entrant
   Failure/Error: ClimateControl.modify(options, &block)
   ThreadError:
     deadlock; recursive locking
   # ./lib/climate_control/modifier.rb:10:in `process'
   # ./lib/climate_control.rb:10:in `modify'
   # ./spec/acceptance/climate_control_spec.rb:131:in `with_modified_env'
   # ./spec/acceptance/climate_control_spec.rb:116:in `block (3 levels) in <top (required)>'
   # ./lib/climate_control/modifier.rb:31:in `call'
   # ./lib/climate_control/modifier.rb:31:in `run_block'
   # ./lib/climate_control/modifier.rb:13:in `block in process'
   # ./lib/climate_control/modifier.rb:10:in `process'
   # ./lib/climate_control.rb:10:in `modify'
   # ./spec/acceptance/climate_control_spec.rb:131:in `with_modified_env'
   # ./spec/acceptance/climate_control_spec.rb:115:in `block (2 levels) in <top (required)>'
   # ./spec/acceptance/climate_control_spec.rb:149:in `block (2 levels) in <top (required)>'
```

Since the lock is here to protect against different threads accessing
ClimateControl at the same time, it's safe for a given thread to make
any changes it wants to the environment as long as it holds the locks,
which is what this patch does.
  • Loading branch information
krallin authored and geoffharcourt committed Mar 31, 2017
1 parent e392d40 commit bd6d7a2
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 1 deletion.
17 changes: 16 additions & 1 deletion lib/climate_control/environment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,25 @@ class Environment

def initialize
@semaphore = Mutex.new
@owner = nil
end

def_delegators :env, :[]=, :to_hash, :[], :delete
def_delegator :@semaphore, :synchronize

def synchronize
if @owner == Thread.current
return yield if block_given?
end

@semaphore.synchronize do
begin
@owner = Thread.current
yield if block_given?
ensure
@owner = nil
end
end
end

private

Expand Down
13 changes: 13 additions & 0 deletions spec/acceptance/climate_control_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,19 @@
expect(ENV["BAZ"]).to be_nil
end

it "is re-entrant" do
ret = with_modified_env(FOO: "foo") do
with_modified_env(BAR: "bar") do
"bar"
end
end

expect(ret).to eq("bar")

expect(ENV["FOO"]).to be_nil
expect(ENV["BAR"]).to be_nil
end

it "raises when the value cannot be assigned properly" do
Thing = Class.new
message = generate_type_error_for_object(Thing.new)
Expand Down

0 comments on commit bd6d7a2

Please sign in to comment.