From 2e1c6fa8b9f53b55efdf2106f5dfbbe5cef8344b Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Sun, 24 Jun 2018 09:59:59 +0100 Subject: [PATCH 1/9] Move concurrency specs into groups --- spec/concurrent/array_spec.rb | 24 +++++++++++++----------- spec/concurrent/hash_spec.rb | 22 ++++++++++++---------- spec/concurrent/set_spec.rb | 24 +++++++++++++----------- 3 files changed, 38 insertions(+), 32 deletions(-) diff --git a/spec/concurrent/array_spec.rb b/spec/concurrent/array_spec.rb index 83450af89..634c0c2ff 100644 --- a/spec/concurrent/array_spec.rb +++ b/spec/concurrent/array_spec.rb @@ -2,18 +2,20 @@ module Concurrent RSpec.describe Array do let!(:ary) { described_class.new } - it 'concurrency' do - (1..Concurrent::ThreadSafe::Test::THREADS).map do |i| - in_thread do - 1000.times do - ary << i - ary.each { |x| x * 2 } - ary.shift - ary.last + context 'concurrency' do + it do + (1..Concurrent::ThreadSafe::Test::THREADS).map do |i| + in_thread do + 1000.times do + ary << i + ary.each { |x| x * 2 } + ary.shift + ary.last + end end - end - end.map(&:join) - expect(ary).to be_empty + end.map(&:join) + expect(ary).to be_empty + end end describe '#slice' do diff --git a/spec/concurrent/hash_spec.rb b/spec/concurrent/hash_spec.rb index 37146048e..68d1c2f23 100644 --- a/spec/concurrent/hash_spec.rb +++ b/spec/concurrent/hash_spec.rb @@ -2,17 +2,19 @@ module Concurrent RSpec.describe Hash do let!(:hsh) { described_class.new } - it 'concurrency' do - (1..Concurrent::ThreadSafe::Test::THREADS).map do |i| - in_thread do - 1000.times do |j| - hsh[i * 1000 + j] = i - expect(hsh[i * 1000 + j]).to eq(i) - expect(hsh.delete(i * 1000 + j)).to eq(i) + context 'concurrency' do + it do + (1..Concurrent::ThreadSafe::Test::THREADS).map do |i| + in_thread do + 1000.times do |j| + hsh[i * 1000 + j] = i + expect(hsh[i * 1000 + j]).to eq(i) + expect(hsh.delete(i * 1000 + j)).to eq(i) + end end - end - end.map(&:join) - expect(hsh).to be_empty + end.map(&:join) + expect(hsh).to be_empty + end end end end diff --git a/spec/concurrent/set_spec.rb b/spec/concurrent/set_spec.rb index 4b2ba36dd..d9c8a649a 100644 --- a/spec/concurrent/set_spec.rb +++ b/spec/concurrent/set_spec.rb @@ -3,18 +3,20 @@ module Concurrent RSpec.describe Set do let!(:set) { described_class.new } - it 'concurrency' do - (1..Concurrent::ThreadSafe::Test::THREADS).map do |i| - in_thread do - 1000.times do - v = i - set << v - expect(set).not_to be_empty - set.delete(v) + context 'concurrency' do + it do + (1..Concurrent::ThreadSafe::Test::THREADS).map do |i| + in_thread do + 1000.times do + v = i + set << v + expect(set).not_to be_empty + set.delete(v) + end end - end - end.map(&:join) - expect(set).to be_empty + end.map(&:join) + expect(set).to be_empty + end end end end From aaa7da15db253e1ae3b61cc8db7df151c21da9b2 Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Sun, 24 Jun 2018 10:21:02 +0100 Subject: [PATCH 2/9] Add specs for Array initialization behavior --- spec/concurrent/array_spec.rb | 63 +++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/spec/concurrent/array_spec.rb b/spec/concurrent/array_spec.rb index 634c0c2ff..c0ecab38f 100644 --- a/spec/concurrent/array_spec.rb +++ b/spec/concurrent/array_spec.rb @@ -2,6 +2,69 @@ module Concurrent RSpec.describe Array do let!(:ary) { described_class.new } + describe '.[]' do + describe 'when initializing with no arguments' do + it do + expect(described_class[]).to be_empty + end + end + + describe 'when initializing with arguments' do + it 'creates an array with the given objects' do + expect(described_class[:hello, :world]).to eq [:hello, :world] + end + end + end + + describe '.new' do + describe 'when initializing with no arguments' do + it do + expect(described_class.new).to be_empty + end + end + + describe 'when initializing with a size argument' do + let(:size) { 3 } + + it 'creates an array with size elements set to nil' do + expect(described_class.new(size)).to eq [nil, nil, nil] + end + + describe 'when initializing with a default value argument' do + let(:default_value) { :ruby } + + it 'creates an array with size elements set to the default value' do + expect(described_class.new(size, default_value)).to eq [:ruby, :ruby, :ruby] + end + end + + describe 'when initializing with a block argument' do + let(:block_argument) { proc { |index| :"ruby#{index}" } } + + it 'creates an array with size elements set to the default value' do + expect(described_class.new(size, &block_argument)).to eq [:ruby0, :ruby1, :ruby2] + end + end + end + + describe 'when initializing with another array as an argument' do + let(:other_array) { [:hello, :world] } + let(:fake_other_array) { double('Fake array', to_ary: other_array) } + + it 'creates a new array' do + expect(described_class.new(other_array)).to_not be other_array + end + + it 'creates an array with the same contents as the other array' do + expect(described_class.new(other_array)).to eq [:hello, :world] + end + + it 'creates an array with the results of calling #to_ary on the other array' do + expect(described_class.new(fake_other_array)).to eq [:hello, :world] + end + end + end + context 'concurrency' do it do (1..Concurrent::ThreadSafe::Test::THREADS).map do |i| From d62d9ebbae0db2edd57e801b929aa1f4b128dbff Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Sun, 24 Jun 2018 11:30:59 +0100 Subject: [PATCH 3/9] Add specs for Hash initialization behavior --- spec/concurrent/hash_spec.rb | 84 ++++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/spec/concurrent/hash_spec.rb b/spec/concurrent/hash_spec.rb index 68d1c2f23..448d214f8 100644 --- a/spec/concurrent/hash_spec.rb +++ b/spec/concurrent/hash_spec.rb @@ -2,6 +2,90 @@ module Concurrent RSpec.describe Hash do let!(:hsh) { described_class.new } + describe '.[]' do + describe 'when initializing with no arguments' do + it do + expect(described_class[]).to be_empty + end + end + + describe 'when initializing with an even number of arguments' do + it 'creates a hash using the odd position arguments as keys and even position arguments as values' do + expect(described_class[:hello, 'hello', :world, 'world']).to eq(hello: 'hello', world: 'world') + end + end + + describe 'when initializing with an array of pairs' do + let(:array_of_pairs) { [[:hello, 'hello'], [:world, 'world']] } + + it 'creates a hash using each pair as a (key, value) pair' do + expect(described_class[array_of_pairs]).to eq(hello: 'hello', world: 'world') + end + end + + describe 'when initializing with another hash as an argument' do + let(:other_hash) { {hello: 'hello', world: 'world'} } + let(:fake_other_hash) { double('Fake hash', to_hash: other_hash) } + + it 'creates a new hash' do + expect(described_class[other_hash]).to_not be other_hash + end + + it 'creates a hash with the same contents as the other hash' do + expect(described_class[other_hash]).to eq(hello: 'hello', world: 'world') + end + + it 'creates a hash with the results of calling #to_hash on the other array' do + expect(described_class[fake_other_hash]).to eq(hello: 'hello', world: 'world') + end + end + end + + describe '.new' do + describe 'when initializing with no arguments' do + it do + expect(described_class.new).to be_empty + end + end + + describe 'when initialized with a default object' do + let(:default_object) { :ruby } + + it 'uses the default object for non-existing keys' do + hash = described_class.new(default_object) + + expect(hash[:hello]).to be :ruby + expect(hash[:world]).to be :ruby + end + end + + describe 'when initialized with a block' do + it 'calls the block for non-existing keys' do + block_calls = [] + + hash = described_class.new do |hash_instance, key| + block_calls << [hash_instance, key] + end + + hash[:hello] + hash[:world] + + expect(block_calls).to eq [[hash, :hello], [hash, :world]] + end + + it 'returns the results of calling the block for non-existing key' do + block_results = ['hello', 'world'] + + hash = described_class.new do + block_results.shift + end + + expect(hash[:hello]).to eq 'hello' + expect(hash[:world]).to eq 'world' + end + end + end + context 'concurrency' do it do (1..Concurrent::ThreadSafe::Test::THREADS).map do |i| From 9be3236859002b2a248849236a1f2d2602e8d63e Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Sun, 24 Jun 2018 11:41:11 +0100 Subject: [PATCH 4/9] Add specs for Set initialization behavior --- spec/concurrent/set_spec.rb | 38 +++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/spec/concurrent/set_spec.rb b/spec/concurrent/set_spec.rb index d9c8a649a..7c69b4bec 100644 --- a/spec/concurrent/set_spec.rb +++ b/spec/concurrent/set_spec.rb @@ -3,6 +3,44 @@ module Concurrent RSpec.describe Set do let!(:set) { described_class.new } + describe '.[]' do + describe 'when initializing with no arguments' do + it do + expect(described_class[]).to be_empty + end + end + + describe 'when initializing with arguments' do + it 'creates a set with the given objects' do + expect(described_class[:hello, :world]).to eq ::Set.new([:hello, :world]) + end + end + end + + describe '.new' do + describe 'when initializing with no arguments' do + it do + expect(described_class.new).to be_empty + end + end + + describe 'when initializing with an enumerable object' do + let(:enumerable_object) { [:hello, :world] } + + it 'creates a set with the contents of the enumerable object' do + expect(described_class.new(enumerable_object)).to eq ::Set.new([:hello, :world]) + end + + describe 'when initializing with a block argument' do + let(:block_argument) { proc { |value| :"#{value}_ruby" } } + + it 'creates a set with the contents of the enumerable object' do + expect(described_class.new(enumerable_object, &block_argument)).to eq ::Set.new([:hello_ruby, :world_ruby]) + end + end + end + end + context 'concurrency' do it do (1..Concurrent::ThreadSafe::Test::THREADS).map do |i| From 3fabacb05e29cbb77df15e661f9ab694ffc34721 Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Sun, 24 Jun 2018 11:45:49 +0100 Subject: [PATCH 5/9] Fix broken initialization arguments on TruffleRuby The `make_synchronized_on_rbx` method is used to intercept the `.new` method on Array/Hash/Set but it broke whenever the `.new` was being used with arguments. (This was shown by the Array/Hash/Set specs that were added in the previous commits being broken on TruffleRuby.) --- lib/concurrent/thread_safe/util/array_hash_rbx.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/concurrent/thread_safe/util/array_hash_rbx.rb b/lib/concurrent/thread_safe/util/array_hash_rbx.rb index 2ee8ce748..602094950 100644 --- a/lib/concurrent/thread_safe/util/array_hash_rbx.rb +++ b/lib/concurrent/thread_safe/util/array_hash_rbx.rb @@ -10,10 +10,9 @@ def _mon_initialize @_monitor = Monitor.new unless @_monitor # avoid double initialisation end - def self.new - obj = super - obj.send(:_mon_initialize) - obj + def initialize(*args) + _mon_initialize + super end end From 17888408f30317375e44166461253bbe8ec80b84 Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Sun, 24 Jun 2018 12:26:55 +0100 Subject: [PATCH 6/9] Fix Array/Hash monitor not being initialized Fixes several broken testcases in the Array and Hash specs. --- lib/concurrent/thread_safe/util/array_hash_rbx.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/concurrent/thread_safe/util/array_hash_rbx.rb b/lib/concurrent/thread_safe/util/array_hash_rbx.rb index 602094950..e413b15f7 100644 --- a/lib/concurrent/thread_safe/util/array_hash_rbx.rb +++ b/lib/concurrent/thread_safe/util/array_hash_rbx.rb @@ -14,6 +14,12 @@ def initialize(*args) _mon_initialize super end + + def self.allocate + obj = super + obj.send(:_mon_initialize) + obj + end end klass.superclass.instance_methods(false).each do |method| From e8ee0d2613756f97c736bfe341ff2b3e602c49cf Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Sun, 24 Jun 2018 12:29:02 +0100 Subject: [PATCH 7/9] Fix another case of Hash monitor not being initialized --- lib/concurrent/thread_safe/util/array_hash_rbx.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/concurrent/thread_safe/util/array_hash_rbx.rb b/lib/concurrent/thread_safe/util/array_hash_rbx.rb index e413b15f7..c9a355511 100644 --- a/lib/concurrent/thread_safe/util/array_hash_rbx.rb +++ b/lib/concurrent/thread_safe/util/array_hash_rbx.rb @@ -20,6 +20,12 @@ def self.allocate obj.send(:_mon_initialize) obj end + + def self.[](*args) + obj = super + obj.send(:_mon_initialize) + obj + end end klass.superclass.instance_methods(false).each do |method| From 04db895168cd446e7b7b8eadf8f6615ad5b5ffb8 Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Sun, 24 Jun 2018 12:50:29 +0100 Subject: [PATCH 8/9] Add developer-friendly error when monitor not initialized As I'm fixing monitor initialization issues, it seems to me that the current approach is a bit whack-a-mole so let's at least prepare a nice error message for users to get in case we're still missing a few corner cases. --- lib/concurrent/thread_safe/util/array_hash_rbx.rb | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/lib/concurrent/thread_safe/util/array_hash_rbx.rb b/lib/concurrent/thread_safe/util/array_hash_rbx.rb index c9a355511..ee129efdf 100644 --- a/lib/concurrent/thread_safe/util/array_hash_rbx.rb +++ b/lib/concurrent/thread_safe/util/array_hash_rbx.rb @@ -41,7 +41,14 @@ def #{method}(*args) else klass.class_eval <<-RUBY, __FILE__, __LINE__ + 1 def #{method}(*args) - @_monitor.synchronize { super } + monitor = @_monitor + + unless monitor + raise("BUG: Internal monitor was not properly initialized. Please report this to the "\ + "concurrent-ruby developers.") + end + + monitor.synchronize { super } end RUBY end From bdcd61fc98f18b68f3e5a3abbb1928ee70b6bc7c Mon Sep 17 00:00:00 2001 From: Petr Chalupa Date: Sun, 1 Jul 2018 20:40:39 +0200 Subject: [PATCH 9/9] Revert previous changes for TruffleRuby * use same approach as for JRuby * use non public TruffleRuby API for simplicity --- lib/concurrent/array.rb | 25 +++++++++++--- lib/concurrent/hash.rb | 27 ++++++++++++--- lib/concurrent/set.rb | 25 +++++++++++--- .../{array_hash_rbx.rb => data_structures.rb} | 34 ++++++++----------- spec/concurrent/array_spec.rb | 4 +-- 5 files changed, 79 insertions(+), 36 deletions(-) rename lib/concurrent/thread_safe/util/{array_hash_rbx.rb => data_structures.rb} (61%) diff --git a/lib/concurrent/array.rb b/lib/concurrent/array.rb index c83f3bb06..f3786056e 100644 --- a/lib/concurrent/array.rb +++ b/lib/concurrent/array.rb @@ -2,7 +2,8 @@ require 'concurrent/thread_safe/util' module Concurrent - if Concurrent.on_cruby? + case + when Concurrent.on_cruby? # Because MRI never runs code in parallel, the existing # non-thread-safe structures should usually work fine. @@ -21,10 +22,10 @@ module Concurrent # may be lost. Use `#concat` instead. # # @see http://ruby-doc.org/core-2.2.0/Array.html Ruby standard library `Array` - class Array < ::Array; + class Array < ::Array end - elsif Concurrent.on_jruby? + when Concurrent.on_jruby? require 'jruby/synchronized' # @!macro concurrent_array @@ -32,15 +33,29 @@ class Array < ::Array include JRuby::Synchronized end - elsif Concurrent.on_rbx? || Concurrent.on_truffleruby? + when Concurrent.on_rbx? require 'monitor' - require 'concurrent/thread_safe/util/array_hash_rbx' + require 'concurrent/thread_safe/util/data_structures' # @!macro concurrent_array class Array < ::Array end ThreadSafe::Util.make_synchronized_on_rbx Concurrent::Array + + when Concurrent.on_truffleruby? + require 'concurrent/thread_safe/util/data_structures' + + # @!macro concurrent_array + class Array < ::Array + end + + ThreadSafe::Util.make_synchronized_on_truffleruby Concurrent::Array + + else + warn 'Possibly unsupported Ruby implementation' + class Array < ::Array + end end end diff --git a/lib/concurrent/hash.rb b/lib/concurrent/hash.rb index 9c952c1fb..3ecc2ff04 100644 --- a/lib/concurrent/hash.rb +++ b/lib/concurrent/hash.rb @@ -2,7 +2,8 @@ require 'concurrent/thread_safe/util' module Concurrent - if Concurrent.on_cruby? + case + when Concurrent.on_cruby? # @!macro [attach] concurrent_hash # @@ -12,10 +13,10 @@ module Concurrent # which takes the lock repeatedly when reading an item. # # @see http://ruby-doc.org/core-2.2.0/Hash.html Ruby standard library `Hash` - class Hash < ::Hash; + class Hash < ::Hash end - elsif Concurrent.on_jruby? + when Concurrent.on_jruby? require 'jruby/synchronized' # @!macro concurrent_hash @@ -23,14 +24,30 @@ class Hash < ::Hash include JRuby::Synchronized end - elsif Concurrent.on_rbx? || Concurrent.on_truffleruby? + when Concurrent.on_rbx? require 'monitor' - require 'concurrent/thread_safe/util/array_hash_rbx' + require 'concurrent/thread_safe/util/data_structures' # @!macro concurrent_hash class Hash < ::Hash end ThreadSafe::Util.make_synchronized_on_rbx Concurrent::Hash + + when Concurrent.on_truffleruby? + require 'concurrent/thread_safe/util/data_structures' + + # @!macro concurrent_hash + class Hash < ::Hash + end + + ThreadSafe::Util.make_synchronized_on_truffleruby Concurrent::Hash + + else + warn 'Possibly unsupported Ruby implementation' + class Hash < ::Hash + end + end end + diff --git a/lib/concurrent/set.rb b/lib/concurrent/set.rb index 7e9956f29..8e72d9bb4 100644 --- a/lib/concurrent/set.rb +++ b/lib/concurrent/set.rb @@ -3,7 +3,8 @@ require 'set' module Concurrent - if Concurrent.on_cruby? + case + when Concurrent.on_cruby? # Because MRI never runs code in parallel, the existing # non-thread-safe structures should usually work fine. @@ -25,7 +26,7 @@ module Concurrent class Set < ::Set; end - elsif Concurrent.on_jruby? + when Concurrent.on_jruby? require 'jruby/synchronized' # @!macro concurrent_Set @@ -33,15 +34,29 @@ class Set < ::Set include JRuby::Synchronized end - elsif Concurrent.on_rbx? || Concurrent.on_truffleruby? + when Concurrent.on_rbx? require 'monitor' - require 'concurrent/thread_safe/util/array_hash_rbx' + require 'concurrent/thread_safe/util/data_structures' # @!macro concurrent_Set class Set < ::Set end - ThreadSafe::Util.make_synchronized_on_rbx Set + ThreadSafe::Util.make_synchronized_on_rbx Concurrent::Set + + when Concurrent.on_truffleruby? + require 'concurrent/thread_safe/util/data_structures' + + # @!macro concurrent_array + class Set < ::Set + end + + ThreadSafe::Util.make_synchronized_on_truffleruby Concurrent::Set + + else + warn 'Possibly unsupported Ruby implementation' + class Set < ::Set + end end end diff --git a/lib/concurrent/thread_safe/util/array_hash_rbx.rb b/lib/concurrent/thread_safe/util/data_structures.rb similarity index 61% rename from lib/concurrent/thread_safe/util/array_hash_rbx.rb rename to lib/concurrent/thread_safe/util/data_structures.rb index ee129efdf..e43d9c50a 100644 --- a/lib/concurrent/thread_safe/util/array_hash_rbx.rb +++ b/lib/concurrent/thread_safe/util/data_structures.rb @@ -6,23 +6,13 @@ module Util def self.make_synchronized_on_rbx(klass) klass.class_eval do private + def _mon_initialize @_monitor = Monitor.new unless @_monitor # avoid double initialisation end - def initialize(*args) - _mon_initialize - super - end - - def self.allocate - obj = super - obj.send(:_mon_initialize) - obj - end - - def self.[](*args) - obj = super + def self.new(*args) + obj = super(*args) obj.send(:_mon_initialize) obj end @@ -42,18 +32,24 @@ def #{method}(*args) klass.class_eval <<-RUBY, __FILE__, __LINE__ + 1 def #{method}(*args) monitor = @_monitor - - unless monitor - raise("BUG: Internal monitor was not properly initialized. Please report this to the "\ - "concurrent-ruby developers.") - end - + monitor or raise("BUG: Internal monitor was not properly initialized. Please report this to the concurrent-ruby developers.") monitor.synchronize { super } end RUBY end end end + + def self.make_synchronized_on_truffleruby(klass) + klass.superclass.instance_methods(false).each do |method| + klass.class_eval <<-RUBY, __FILE__, __LINE__ + 1 + def #{method}(*args, &block) + # TODO (pitr-ch 01-Jul-2018): don't use internal TruffleRuby APIs + Truffle::System.synchronized(self) { super(*args, &block) } + end + RUBY + end + end end end end diff --git a/spec/concurrent/array_spec.rb b/spec/concurrent/array_spec.rb index c0ecab38f..f79f8e077 100644 --- a/spec/concurrent/array_spec.rb +++ b/spec/concurrent/array_spec.rb @@ -68,7 +68,7 @@ module Concurrent context 'concurrency' do it do (1..Concurrent::ThreadSafe::Test::THREADS).map do |i| - in_thread do + in_thread(ary) do |ary| 1000.times do ary << i ary.each { |x| x * 2 } @@ -82,7 +82,7 @@ module Concurrent end describe '#slice' do - # This is mostly relevant on Rubinius and Truffle + # This is mostly relevant on Rubinius and TruffleRuby it 'correctly initializes the monitor' do ary.concat([0, 1, 2, 3, 4, 5, 6, 7, 8])