Merge pull request #734 from ivoanjo/fix-array-hash-set-constructors-truffleruby

Fix Array/Hash/Set construction broken on TruffleRuby

Author: pitr-ch

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,26 +22,40 @@ 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
     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
 

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,25 +13,41 @@ 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
     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
+

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,23 +26,37 @@ module Concurrent
     class Set < ::Set;
     end
 
-  elsif Concurrent.on_jruby?
+  when Concurrent.on_jruby?
     require 'jruby/synchronized'
 
     # @!macro concurrent_Set
     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
 

lib/concurrent/thread_safe/util/array_hash_rbx.rb renamed to lib/concurrent/thread_safe/util/data_structures.rb

@@ -6,12 +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 self.new
-            obj = super
+          def self.new(*args)
+            obj = super(*args)
             obj.send(:_mon_initialize)
             obj
           end
@@ -30,12 +31,25 @@ def #{method}(*args)
           else
             klass.class_eval <<-RUBY, __FILE__, __LINE__ + 1
               def #{method}(*args)
-                @_monitor.synchronize { super }
+                monitor = @_monitor
+                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

spec/concurrent/array_spec.rb

@@ -2,22 +2,87 @@ 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
+    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
-      end.map(&:join)
-      expect(ary).to be_empty
+
+        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|
+          in_thread(ary) do |ary|
+            1000.times do
+              ary << i
+              ary.each { |x| x * 2 }
+              ary.shift
+              ary.last
+            end
+          end
+        end.map(&:join)
+        expect(ary).to be_empty
+      end
     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])