Add a Monitor to ModelSchema#load_schema

[Vikrant Chaudhary, David Abdemoulaie, Matthew Draper]

3 files changed, 56 insertions, 3 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 907dd894cd..d17bbf80ca 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,9 @@
+*   Loading model schema from database is now thread-safe.
+
+    Fixes #28589.
+
+    *Vikrant Chaudhary*, *David Abdemoulaie*
+
 *   Add `ActiveRecord::Base#cache_version` to support recyclable cache keys via the new versioned entries
     in `ActiveSupport::Cache`. This also means that `ActiveRecord::Base#cache_key` will now return a stable key
     that does not include a timestamp any more.
diff --git a/activerecord/lib/active_record/model_schema.rb b/activerecord/lib/active_record/model_schema.rb
index 54216caaaf..5095cdfe9f 100644
--- a/activerecord/lib/active_record/model_schema.rb
+++ b/activerecord/lib/active_record/model_schema.rb
@@ -1,3 +1,5 @@
+require "monitor"
+
 module ActiveRecord
   module ModelSchema
     extend ActiveSupport::Concern
@@ -152,6 +154,8 @@ module ActiveRecord
       self.inheritance_column = "type"
 
       delegate :type_for_attribute, to: :class
+
+      initialize_load_schema_monitor
     end
 
     # Derives the join table name for +first_table+ and +second_table+. The
@@ -435,15 +439,27 @@ module ActiveRecord
         initialize_find_by_cache
       end
 
+      protected
+
+        def initialize_load_schema_monitor
+          @load_schema_monitor = Monitor.new
+        end
+
       private
 
+        def inherited(child_class)
+          super
+          child_class.initialize_load_schema_monitor
+        end
+
         def schema_loaded?
-          defined?(@columns_hash) && @columns_hash
+          defined?(@schema_loaded) && @schema_loaded
         end
 
         def load_schema
-          unless schema_loaded?
-            load_schema!
+          return if schema_loaded?
+          @load_schema_monitor.synchronize do
+            load_schema! unless defined?(@columns_hash) && @columns_hash
           end
         end
 
@@ -457,6 +473,8 @@ module ActiveRecord
               user_provided_default: false
             )
           end
+
+          @schema_loaded = true
         end
 
         def reload_schema_from_cache
@@ -470,6 +488,7 @@ module ActiveRecord
           @attributes_builder = nil
           @columns = nil
           @columns_hash = nil
+          @schema_loaded = false
           @attribute_names = nil
           @yaml_encoder = nil
           direct_descendants.each do |descendant|
diff --git a/activerecord/test/cases/serialized_attribute_test.rb b/activerecord/test/cases/serialized_attribute_test.rb
index 673392b4c4..e1bdaab5cf 100644
--- a/activerecord/test/cases/serialized_attribute_test.rb
+++ b/activerecord/test/cases/serialized_attribute_test.rb
@@ -349,4 +349,32 @@ class SerializedAttributeTest < ActiveRecord::TestCase
     topic.foo
     refute topic.changed?
   end
+
+  def test_serialized_attribute_works_under_concurrent_initial_access
+    model = Topic.dup
+
+    topic = model.last
+    topic.update group: "1"
+
+    model.serialize :group, JSON
+    model.reset_column_information
+
+    # This isn't strictly necessary for the test, but a little bit of
+    # knowledge of internals allows us to make failures far more likely.
+    model.define_singleton_method(:define_attribute) do |*args|
+      Thread.pass
+      super(*args)
+    end
+
+    threads = 4.times.map do
+      Thread.new do
+        topic.reload.group
+      end
+    end
+
+    # All the threads should retrieve the value knowing it is JSON, and
+    # thus decode it. If this fails, some threads will instead see the
+    # raw string ("1"), or raise an exception.
+    assert_equal [1] * threads.size, threads.map(&:value)
+  end
 end