fix: avoid race condition in singleton instance method

[zvkemp]

||= is not an atomic operation, so two simultaneous callers may be given different instrumentation instances. I do not know whether this ever practically matters, but if the intention is that these should be singletons, then that condition should be avoided by wrapping the assignment portion in a synchronize block.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: zvkemp / name: Zach Kemp (fcd48b5)

[zvkemp]

@kaylareopelle In response to your question as to whether this is testable, here's a contrived example of a singleton that returns multiple instance when more than one thread is requesting it, and the corresponding fix applied to another test. Thread.pass hints that the VM scheduler should activate another thread, but any sort of latency in initialize can have the same effect (e.g. sleep(0.01))

require 'minitest/autorun'

class SingletonClass
  def initialize
    Thread.pass
  end

  class << self
    def instance
      @instance ||= new
    end
  end
end

class SafeSingletonClass
  MUTEX = Thread::Mutex.new

  def initialize
    Thread.pass
  end

  class << self
    def instance
      @instance || MUTEX.synchronize { @instance ||= new }
    end
  end
end

describe 'singleton acquisition' do
  it 'fails' do
    ids = 2.times.map do
      Thread.new { SingletonClass.instance.object_id }
    end.map(&:join).map(&:value)

    _(ids.uniq.count).must_equal(1)
  end

  it 'works' do
    ids = 2.times.map do
      Thread.new { SafeSingletonClass.instance.object_id }
    end.map(&:join).map(&:value)

    _(ids.uniq.count).must_equal(1)
  end
end

As to how this might be incorporated into this repo — I could probably structure the test around a test-only subclass of Instumentation::Base that has roughly the same characteristic?

[arielvalentin]

Instrumentations should be initialized during application boot time, and I am skeptical of programs that perform multi-threaded initialization or Ruby applications.

Do you have any examples or bug reports of this happening in applications?

[zvkemp]

Do you have any examples or bug reports of this happening in applications?

I haven't seen bug reports for this library specifically. The intention here is to put the guarantee somewhere — in most cases initialization should happen at boot in a single thread, but that doesn't seem to be enforced or otherwise guaranteed (and if it was, it would be in a downstream library, i.e. the sdk, which includes this library as a dependency). The instance method implies a contract that should be difficult to unintentionally violate, and within the scope of this repo this change achieves that.

One thing I did not consider until just this moment is that putting the mutex into a constant forces it to be shared amongst all of the sub-classes; in practice that is probably fine but isn't completely correct. It could introduce deadlocks if anything refers to another subclass's instance during initialize. (edit: changed to class-level instance variables)