Skip to content

Commit

Permalink
Kind fetcher locks are not fully thread-safe
Browse files Browse the repository at this point in the history
In case of extensive concurrent usage, the mutex handed over to two threads under same key may differ as illustrated below:

```ruby
require 'concurrent'

10000.times do
  kind_fetcher_locks = Concurrent::Hash.new { |hash, key| hash[key] = Mutex.new }

  refs = Set.new
  100.times.map do |i|
    Thread.new { refs << kind_fetcher_locks[i % 50].object_id }
  end.each(&:join)

  raise "Not 50 but #{refs.count}" unless refs.size == 50
end
```

this can lead to really weird issues.

Works when fixed as above:

```ruby
require 'concurrent'

10000.times do
  mutex = Mutex.new
  # kind_fetcher_locks = Concurrent::Hash.new { |hash, key| hash[key] = Mutex.new }

  kind_fetcher_locks = Concurrent::Hash.new do |hash, key|
    mutex.synchronize do
      break hash[key] if hash.key?(key)
      hash[key] = Mutex.new
    end
  end

  refs = Set.new
  100.times.map do |i|
    Thread.new { refs << kind_fetcher_locks[i % 50].object_id }
  end.each(&:join)

  raise "Not 50 but #{refs.count}" unless refs.size == 50
end
```

ref ruby-concurrency/concurrent-ruby#970
  • Loading branch information
mensfeld authored Nov 21, 2022
1 parent c093e5f commit 4839a03
Showing 1 changed file with 7 additions and 2 deletions.
9 changes: 7 additions & 2 deletions lib/krane/resource_cache.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,13 @@ class ResourceCache

def initialize(task_config)
@task_config = task_config

@kind_fetcher_locks = Concurrent::Hash.new { |hash, key| hash[key] = Mutex.new }
@mutex = Mutex.new
@kind_fetcher_locks = Concurrent::Hash.new do |hash, key|
@mutex.synchronize do
break hash[key] if hash.key?(key)
hash[key] = Mutex.new
end
end
@data = Concurrent::Hash.new
@kubectl = Kubectl.new(task_config: @task_config, log_failure_by_default: false)
end
Expand Down

0 comments on commit 4839a03

Please sign in to comment.