Wrap entire compile step with mutex (#2132)

* Wrap entire compile step with mutex

Currently we only wrap the redefinition of templates with a mutex. This
has largely been fine, but recent changes have highlighted that being
overly specific with the mutex still leads to race conditions that are
complex to debug or test for.

This change wraps the entire compile step with a mutex to help avoid
introducing any race conditions during the compile step.

I tested this change against the last race condition bug and validated
that the demo app I setup locally still runs as expected.

* Fix linter, add changelog entry

* Make changelog a sentence

* fix allocations count

---------

Co-authored-by: Joel Hawksley <[email protected]>
Co-authored-by: Joel Hawksley <[email protected]>

Author: BlakeWilliams

docs/CHANGELOG.md

@@ -10,6 +10,10 @@ nav_order: 5
 
 ## main
 
+* Wrap entire compile step in a mutex to make it more resilient to race conditions.
+
+    *Blake Williams*
+
 * Add [Niva]([niva.co](https://www.niva.co/)) to companies who use `ViewComponent`.
 
     *Daniel Vu Dao*

lib/view_component/compiler.rb

@@ -12,7 +12,7 @@ class Compiler
 
     def initialize(component)
       @component = component
-      @redefinition_lock = Mutex.new
+      @lock = Mutex.new
       @rendered_templates = Set.new
     end
 
@@ -24,30 +24,36 @@ def compile(raise_errors: false, force: false)
       return if compiled? && !force
       return if @component == ViewComponent::Base
 
-      gather_templates
+      @lock.synchronize do
+        # this check is duplicated so that concurrent compile calls can still
+        # early exit
+        return if compiled? && !force
 
-      if self.class.development_mode && @templates.any?(&:requires_compiled_superclass?)
-        @component.superclass.compile(raise_errors: raise_errors)
-      end
+        gather_templates
 
-      if template_errors.present?
-        raise TemplateError.new(template_errors) if raise_errors
+        if self.class.development_mode && @templates.any?(&:requires_compiled_superclass?)
+          @component.superclass.compile(raise_errors: raise_errors)
+        end
 
-        # this return is load bearing, and prevents the component from being considered "compiled?"
-        return false
-      end
+        if template_errors.present?
+          raise TemplateError.new(template_errors) if raise_errors
 
-      if raise_errors
-        @component.validate_initialization_parameters!
-        @component.validate_collection_parameter!
-      end
+          # this return is load bearing, and prevents the component from being considered "compiled?"
+          return false
+        end
 
-      define_render_template_for
+        if raise_errors
+          @component.validate_initialization_parameters!
+          @component.validate_collection_parameter!
+        end
+
+        define_render_template_for
 
-      @component.register_default_slots
-      @component.build_i18n_backend
+        @component.register_default_slots
+        @component.build_i18n_backend
 
-      CompileCache.register(@component)
+        CompileCache.register(@component)
+      end
     end
 
     def renders_template_for?(variant, format)
@@ -60,9 +66,7 @@ def renders_template_for?(variant, format)
 
     def define_render_template_for
       @templates.each do |template|
-        @redefinition_lock.synchronize do
-          template.compile_to_component
-        end
+        template.compile_to_component
       end
 
       method_body =
@@ -93,14 +97,12 @@ def define_render_template_for
           out << "else\n  #{templates.find { _1.variant.nil? && _1.default_format? }.safe_method_name}\nend"
         end
 
-      @redefinition_lock.synchronize do
-        @component.silence_redefinition_of_method(:render_template_for)
-        @component.class_eval <<-RUBY, __FILE__, __LINE__ + 1
-        def render_template_for(variant = nil, format = nil)
-          #{method_body}
-        end
-        RUBY
+      @component.silence_redefinition_of_method(:render_template_for)
+      @component.class_eval <<-RUBY, __FILE__, __LINE__ + 1
+      def render_template_for(variant = nil, format = nil)
+        #{method_body}
       end
+      RUBY
     end
 
     def template_errors

test/sandbox/test/rendering_test.rb

@@ -15,7 +15,7 @@ def test_render_inline_allocations
     ViewComponent::CompileCache.cache.delete(MyComponent)
     MyComponent.ensure_compiled
 
-    assert_allocations("3.4.0" => 107, "3.3.5" => 116, "3.3.0" => 129, "3.2.5" => 115, "3.1.6" => 115, "3.0.7" => 125) do
+    assert_allocations("3.4.0" => 110, "3.3.5" => 116, "3.3.0" => 129, "3.2.5" => 115, "3.1.6" => 115, "3.0.7" => 125) do
       render_inline(MyComponent.new)
     end