Extract safe initialization and use it for MutexAtomic{Reference,Fixnum,Boolean}

* The previous logic risked a thread seeing @__Lock__ being nil
  if the object was published racily.

Author: eregon

ext/concurrent-ruby/com/concurrent_ruby/ext/SynchronizationLibrary.java

@@ -55,12 +55,6 @@ private static boolean supportsFences() {
         }
     }
 
-    private static final ObjectAllocator JRUBY_OBJECT_ALLOCATOR = new ObjectAllocator() {
-        public IRubyObject allocate(Ruby runtime, RubyClass klazz) {
-            return new JRubyObject(runtime, klazz);
-        }
-    };
-
     private static final ObjectAllocator OBJECT_ALLOCATOR = new ObjectAllocator() {
         public IRubyObject allocate(Ruby runtime, RubyClass klazz) {
             return new Object(runtime, klazz);
@@ -87,10 +81,7 @@ public void load(Ruby runtime, boolean wrap) throws IOException {
         RubyModule jrubyAttrVolatileModule = synchronizationModule.defineModuleUnder("JRubyAttrVolatile");
         jrubyAttrVolatileModule.defineAnnotatedMethods(JRubyAttrVolatile.class);
 
-        defineClass(runtime, synchronizationModule, "AbstractObject", "JRubyObject",
-                JRubyObject.class, JRUBY_OBJECT_ALLOCATOR);
-
-        defineClass(runtime, synchronizationModule, "JRubyObject", "Object",
+        defineClass(runtime, synchronizationModule, "AbstractObject", "Object",
                 Object.class, OBJECT_ALLOCATOR);
 
         defineClass(runtime, synchronizationModule, "Object", "AbstractLockableObject",
@@ -143,8 +134,8 @@ public static class JRubyAttrVolatile {
         // attempt to avoid code elimination.
         private static volatile int volatileField;
 
-        @JRubyMethod(name = "full_memory_barrier", visibility = Visibility.PUBLIC)
-        public static IRubyObject fullMemoryBarrier(ThreadContext context, IRubyObject self) {
+        @JRubyMethod(name = "full_memory_barrier", visibility = Visibility.PUBLIC, module = true)
+        public static IRubyObject fullMemoryBarrier(ThreadContext context, IRubyObject module) {
             // Prevent reordering of ivar writes with publication of this instance
             if (!FULL_FENCE) {
                 // Assuming that following volatile read and write is not eliminated it simulates fullFence.
@@ -195,16 +186,8 @@ public static IRubyObject InstanceVariableSetVolatile(
         }
     }
 
-    @JRubyClass(name = "JRubyObject", parent = "AbstractObject")
-    public static class JRubyObject extends RubyObject {
-
-        public JRubyObject(Ruby runtime, RubyClass metaClass) {
-            super(runtime, metaClass);
-        }
-    }
-
-    @JRubyClass(name = "Object", parent = "JRubyObject")
-    public static class Object extends JRubyObject {
+    @JRubyClass(name = "Object", parent = "AbstractObject")
+    public static class Object extends RubyObject {
 
         public Object(Ruby runtime, RubyClass metaClass) {
             super(runtime, metaClass);
@@ -220,7 +203,7 @@ public AbstractLockableObject(Ruby runtime, RubyClass metaClass) {
     }
 
     @JRubyClass(name = "JRubyLockableObject", parent = "AbstractLockableObject")
-    public static class JRubyLockableObject extends JRubyObject {
+    public static class JRubyLockableObject extends AbstractLockableObject {
 
         public JRubyLockableObject(Ruby runtime, RubyClass metaClass) {
             super(runtime, metaClass);

lib/concurrent-ruby/concurrent/atomic/mutex_atomic_boolean.rb

@@ -1,16 +1,18 @@
+require 'concurrent/synchronization/safe_initialization'
+
 module Concurrent
 
   # @!macro atomic_boolean
   # @!visibility private
   # @!macro internal_implementation_note
   class MutexAtomicBoolean
+    extend Concurrent::Synchronization::SafeInitialization
 
     # @!macro atomic_boolean_method_initialize
     def initialize(initial = false)
       super()
       @__Lock__ = ::Mutex.new
-      # This synchronize should be good enough to ensure safe initialization
-      synchronize { ns_initialize(initial) }
+      @value = !!initial
     end
 
     # @!macro atomic_boolean_method_value_get
@@ -54,11 +56,6 @@ def synchronize
       end
     end
 
-    # @!visibility private
-    def ns_initialize(initial)
-      @value = !!initial
-    end
-
     private
 
     # @!visibility private

lib/concurrent-ruby/concurrent/atomic/mutex_atomic_fixnum.rb

@@ -1,3 +1,4 @@
+require 'concurrent/synchronization/safe_initialization'
 require 'concurrent/utility/native_integer'
 
 module Concurrent
@@ -6,13 +7,13 @@ module Concurrent
   # @!visibility private
   # @!macro internal_implementation_note
   class MutexAtomicFixnum
+    extend Concurrent::Synchronization::SafeInitialization
 
     # @!macro atomic_fixnum_method_initialize
     def initialize(initial = 0)
       super()
       @__Lock__ = ::Mutex.new
-      # This synchronize should be good enough to ensure safe initialization
-      synchronize { ns_initialize(initial) }
+      ns_set(initial)
     end
 
     # @!macro atomic_fixnum_method_value_get
@@ -69,11 +70,6 @@ def synchronize
       end
     end
 
-    # @!visibility private
-    def ns_initialize(initial)
-      ns_set(initial)
-    end
-
     private
 
     # @!visibility private

lib/concurrent-ruby/concurrent/atomic_reference/mutex_atomic.rb

@@ -1,8 +1,11 @@
+require 'concurrent/synchronization/safe_initialization'
+
 module Concurrent
 
   # @!visibility private
   # @!macro internal_implementation_note
   class MutexAtomicReference
+    extend Concurrent::Synchronization::SafeInitialization
     include AtomicDirectUpdate
     include AtomicNumericCompareAndSetWrapper
     alias_method :compare_and_swap, :compare_and_set
@@ -11,8 +14,7 @@ class MutexAtomicReference
     def initialize(value = nil)
       super()
       @__Lock__ = ::Mutex.new
-      # This synchronize should be good enough to ensure safe initialization
-      synchronize { ns_initialize(value) }
+      @value = value
     end
 
     # @!macro atomic_reference_method_get
@@ -59,10 +61,5 @@ def synchronize
         @__Lock__.synchronize { yield }
       end
     end
-
-    # @!visibility private
-    def ns_initialize(value)
-      @value = value
-    end
   end
 end

lib/concurrent-ruby/concurrent/synchronization/abstract_object.rb

@@ -4,10 +4,8 @@ module Synchronization
     # @!visibility private
     # @!macro internal_implementation_note
     class AbstractObject
-
-      # @abstract has to be implemented based on Ruby runtime
       def initialize
-        raise NotImplementedError
+        # nothing to do
       end
 
       # @!visibility private

lib/concurrent-ruby/concurrent/synchronization/full_memory_barrier.rb

@@ -0,0 +1,27 @@
+module Concurrent
+  module Synchronization
+    case
+    when Concurrent.on_cruby?
+      def self.full_memory_barrier
+        # relying on undocumented behavior of CRuby, GVL acquire has lock which ensures visibility of ivars
+        # https://github.com/ruby/ruby/blob/ruby_2_2/thread_pthread.c#L204-L211
+      end
+
+    when Concurrent.on_jruby?
+      require 'concurrent/utility/native_extension_loader'
+      def self.full_memory_barrier
+        JRubyAttrVolatile.full_memory_barrier
+      end
+
+    when Concurrent.on_truffleruby?
+      def self.full_memory_barrier
+        TruffleRuby.full_memory_barrier
+      end
+
+    else
+      warn 'Possibly unsupported Ruby implementation'
+      def self.full_memory_barrier
+      end
+    end
+  end
+end

lib/concurrent-ruby/concurrent/synchronization/jruby_object.rb

@@ -2,7 +2,6 @@ module Concurrent
   module Synchronization
 
     if Concurrent.on_jruby? && Concurrent.java_extensions_loaded?
-
       # @!visibility private
       module JRubyAttrVolatile
         def self.included(base)
@@ -30,16 +29,7 @@ def #{name}=(value)
           end
         end
       end
-
-      # @!visibility private
-      # @!macro internal_implementation_note
-      class JRubyObject < AbstractObject
-        include JRubyAttrVolatile
-
-        def initialize
-          # nothing to do
-        end
-      end
     end
+
   end
 end

lib/concurrent-ruby/concurrent/synchronization/mri_object.rb

@@ -24,21 +24,7 @@ def #{name}=(value)
           names.map { |n| [n, :"#{n}="] }.flatten
         end
       end
-
-      def full_memory_barrier
-        # relying on undocumented behavior of CRuby, GVL acquire has lock which ensures visibility of ivars
-        # https://github.com/ruby/ruby/blob/ruby_2_2/thread_pthread.c#L204-L211
-      end
     end
 
-    # @!visibility private
-    # @!macro internal_implementation_note
-    class MriObject < AbstractObject
-      include MriAttrVolatile
-
-      def initialize
-        # nothing to do
-      end
-    end
   end
 end

lib/concurrent-ruby/concurrent/synchronization/object.rb

@@ -1,28 +1,17 @@
+require 'concurrent/synchronization/safe_initialization'
+require 'concurrent/synchronization/volatile'
 require 'concurrent/atomic/atomic_reference'
 
 module Concurrent
   module Synchronization
 
-    # @!visibility private
-    # @!macro internal_implementation_note
-    ObjectImplementation = case
-                           when Concurrent.on_cruby?
-                             MriObject
-                           when Concurrent.on_jruby?
-                             JRubyObject
-                           when Concurrent.on_truffleruby?
-                             TruffleRubyObject
-                           else
-                             warn 'Possibly unsupported Ruby implementation'
-                             MriObject
-                           end
-    private_constant :ObjectImplementation
-
     # Abstract object providing final, volatile, ans CAS extensions to build other concurrent abstractions.
     # - final instance variables see {Object.safe_initialization!}
     # - volatile instance variables see {Object.attr_volatile}
     # - volatile instance variables see {Object.attr_atomic}
-    class Object < ObjectImplementation
+    class Object < AbstractObject
+      include Volatile
+
       # TODO make it a module if possible
 
       # @!method self.attr_volatile(*names)
@@ -38,36 +27,12 @@ def initialize
         __initialize_atomic_fields__
       end
 
-      # By calling this method on a class, it and all its children are marked to be constructed safely. Meaning that
-      # all writes (ivar initializations) are made visible to all readers of newly constructed object. It ensures
-      # same behaviour as Java's final fields.
-      # @example
-      #   class AClass < Concurrent::Synchronization::Object
-      #     safe_initialization!
-      #
-      #     def initialize
-      #       @AFinalValue = 'value' # published safely, does not have to be synchronized
-      #     end
-      #   end
-      # @return [true]
       def self.safe_initialization!
-        # define only once, and not again in children
-        return if safe_initialization?
-
-        # @!visibility private
-        def self.new(*args, &block)
-          object = super(*args, &block)
-        ensure
-          object.full_memory_barrier if object
-        end
-
-        @safe_initialization = true
+        extend SafeInitialization
       end
 
-      # @return [true, false] if this class is safely initialized.
       def self.safe_initialization?
-        @safe_initialization = false unless defined? @safe_initialization
-        @safe_initialization || (superclass.respond_to?(:safe_initialization?) && superclass.safe_initialization?)
+        self.singleton_class < SafeInitialization
       end
 
       # For testing purposes, quite slow. Injects assert code to new method which will raise if class instance contains

lib/concurrent-ruby/concurrent/synchronization/safe_initialization.rb

@@ -0,0 +1,36 @@
+require 'concurrent/synchronization/full_memory_barrier'
+
+module Concurrent
+  module Synchronization
+
+    # @!visibility private
+    # @!macro internal_implementation_note
+    #
+    # By extending this module, a class and all its children are marked to be constructed safely. Meaning that
+    # all writes (ivar initializations) are made visible to all readers of newly constructed object. It ensures
+    # same behaviour as Java's final fields.
+    #
+    # Due to using Kernel#extend, the module is not included again if already present in the ancestors,
+    # which avoids extra overhead.
+    #
+    # @example
+    #   class AClass < Concurrent::Synchronization::Object
+    #     extend Concurrent::Synchronization::SafeInitialization
+    #
+    #     def initialize
+    #       @AFinalValue = 'value' # published safely, #foo will never return nil
+    #     end
+    #
+    #     def foo
+    #       @AFinalValue
+    #     end
+    #   end
+    module SafeInitialization
+      def new(*args, &block)
+        super(*args, &block)
+      ensure
+        Concurrent::Synchronization.full_memory_barrier
+      end
+    end
+  end
+end