From 18dc263a0f88a4f3304740cd061948f0b9f85826 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    | 24 +++++++++++++++++++
 2 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/instrumentation/base/lib/opentelemetry/instrumentation/base.rb b/instrumentation/base/lib/opentelemetry/instrumentation/base.rb
index ddf828955..ac1e6b8c4 100644
--- a/instrumentation/base/lib/opentelemetry/instrumentation/base.rb
+++ b/instrumentation/base/lib/opentelemetry/instrumentation/base.rb
@@ -69,8 +69,9 @@ class << self
           integer: ->(v) { v.is_a?(Integer) },
           string: ->(v) { v.is_a?(String) }
         }.freeze
+        SINGLETON_MUTEX = Thread::Mutex.new
 
-        private_constant :NAME_REGEX, :VALIDATORS
+        private_constant :NAME_REGEX, :VALIDATORS, :SINGLETON_MUTEX
 
         private :new
 
@@ -163,8 +164,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..76bd05e24 100644
--- a/instrumentation/base/test/instrumentation/base_test.rb
+++ b/instrumentation/base/test/instrumentation/base_test.rb
@@ -59,9 +59,33 @@ 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
+
     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
   end
 
   describe '.option' do