From fcd48b5136c5f5caf36fa97c4762af7fd9bfd695 Mon Sep 17 00:00:00 2001
From: zvkemp <zvkemp@gmail.com>
Date: Mon, 16 Dec 2024 15:08:38 -0500
Subject: [PATCH] fix: avoid race condition in singleton `instance` method

---
 .../lib/opentelemetry/instrumentation/base.rb |  9 +++-
 .../base/test/instrumentation/base_test.rb    | 44 +++++++++++++++++++
 2 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/instrumentation/base/lib/opentelemetry/instrumentation/base.rb b/instrumentation/base/lib/opentelemetry/instrumentation/base.rb
index ddf828955..def61db09 100644
--- a/instrumentation/base/lib/opentelemetry/instrumentation/base.rb
+++ b/instrumentation/base/lib/opentelemetry/instrumentation/base.rb
@@ -60,6 +60,8 @@ module Instrumentation
     # '::' replaced by underscores, OPENTELEMETRY shortened to OTEL_{LANG}, and '_ENABLED' appended.
     # For example: OTEL_RUBY_INSTRUMENTATION_SINATRA_ENABLED = false.
     class Base
+      @singleton_mutex = Thread::Mutex.new
+
       class << self
         NAME_REGEX = /^(?:(?<namespace>[a-zA-Z0-9_:]+):{2})?(?<classname>[a-zA-Z0-9_]+)$/
         VALIDATORS = {
@@ -75,6 +77,7 @@ class << self
         private :new
 
         def inherited(subclass) # rubocop:disable Lint/MissingSuper
+          subclass.instance_exec { @singleton_mutex = Thread::Mutex.new }
           OpenTelemetry::Instrumentation.registry.register(subclass)
         end
 
@@ -163,8 +166,10 @@ def option(name, default:, validate:)
         end
 
         def instance
-          @instance ||= new(instrumentation_name, instrumentation_version, install_blk,
-                            present_blk, compatible_blk, options)
+          @instance || @singleton_mutex.synchronize do
+            @instance ||= new(instrumentation_name, instrumentation_version, install_blk,
+                              present_blk, compatible_blk, options)
+          end
         end
 
         private
diff --git a/instrumentation/base/test/instrumentation/base_test.rb b/instrumentation/base/test/instrumentation/base_test.rb
index c58bbe500..9382df329 100644
--- a/instrumentation/base/test/instrumentation/base_test.rb
+++ b/instrumentation/base/test/instrumentation/base_test.rb
@@ -59,9 +59,53 @@ def initialize(*args)
   end
 
   describe '.instance' do
+    let(:instrumentation) do
+      Class.new(OpenTelemetry::Instrumentation::Base) do
+        instrumentation_name 'test_instrumentation'
+        instrumentation_version '0.1.1'
+
+        def initialize(*args)
+          # Simulate latency by hinting the VM should switch tasks
+          # (this can also be accomplished by something like `sleep(0.1)`).
+          # This replicates the worst-case scenario when using default assignment
+          # to obtain a singleton, i.e. that the scheduler switches threads between
+          # the nil check and object initialization.
+          Thread.pass
+          super
+        end
+      end
+    end
+
+    let(:other_instrumentation) do
+      aux_instrumentation = instrumentation
+      Class.new(OpenTelemetry::Instrumentation::Base) do
+        instrumentation_name 'test_instrumentation'
+        instrumentation_version '0.1.1'
+
+        define_method(:aux_instrumentation) { aux_instrumentation }
+
+        def initialize(*)
+          aux_instrumentation.instance
+
+          super
+        end
+      end
+    end
+
     it 'returns an instance' do
       _(instrumentation.instance).must_be_instance_of(instrumentation)
     end
+
+    it 'returns the same singleton instance to every thread' do
+      object_ids = Array.new(2).map { Thread.new { instrumentation.instance } }
+                        .map { |thr| thr.join.value }
+
+      _(object_ids.uniq.count).must_equal(1)
+    end
+
+    it 'can refer to other instances in initialize without deadlocking' do
+      other_instrumentation.instance
+    end
   end
 
   describe '.option' do