fix: Correctly set mutex

[ixti]

What does this PR do?

Fixes Mutex assignment to be thread-safe.

Motivation

Tried to find what might be causing #2045 and found (unfortunately unrelated) but somewhat problematic code. We've tried this patch in production, unfortunately as I said, it's not fixing #2045, but still worth fixing.

Additional Notes

This is a simple demo, why @mutex ||= Mutex.new is a bad idea. Save the below ruby script as demo.rb:

    module BeforePatch
      def mutex
        @mutex ||= Mutex.new
      end
    end

    module AfterPatch
      def self.included(base)
        base.prepend(PrependMethods)
      end

      module PrependMethods
        def initialize(...)
          @mutex = Mutex.new
          super(...)
        end

        attr_reader :mutex
      end
    end

    class Demo
      include(ENV["FIXED"] ? AfterPatch : BeforePatch)
    end

    demo    = Demo.new
    results = []
    workers = Array.new(32) do
      Thread.new do
        results << demo.mutex.object_id
      end
    end

    workers.each(&:join)
    puts results.tally

Run:

    while true; do ruby demo.rb; done

At some point you will see that there are more than one mutex. Now, run:

    while true; do FIXED=1 ruby demo.rb; done

There should be no such issue.

How to test the change?

It's not very feasible to automatically test this. But the change is small enough.

[delner]

Is there a reason this needs to be done in the constructor?

Generally speaking its not great to prepend onto initialize, because not all objects have the same constructor signature. It introduces a kind of unspoken coupling (expects implementing objects to have a very specific constructor) and makes the module inflexible.

I suppose if you're doing a wildcard super, I guess it technically does work with any constructor. Just seems a bit weird. Then again, maybe its the original design is what's weird... I probably would have to take credit for that 😅

[delner]

Attempting to answer my own question, I'm going to guess this has something to do with lazy initialization not being sufficient if that field is accessed concurrently prior to initialization...

[delner]

FWIW the breakage I'm seeing in CI right now is related to the constructor definition:

68.2) Failure/Error:
              def initialize(&block)
                @task = block
              end
            
            ArgumentError:
              wrong number of arguments (given 1, expected 0)
            # ./lib/datadog/core/worker.rb:8:in `initialize'

But it's only on Ruby 2.1-2.4. Probably the kwargs, so we may need to introduce some compatibility checks there.

[ixti]

Attempting to answer my own question, I'm going to guess this has something to do with lazy initialization not being sufficient if that field is accessed concurrently prior to initialization...

Yeah, there's a chance that one thread will use one mutex instance, and then another will use another one. If we can move mutex into a constant — that would solve the issue entirely :D but can cause an issue with performance (haven't looked into details how those classes are used).

[ixti]

@delner I have an alternative idea how to ensure all threads are using correct mutex, not sure how will you like the idea though:

module Async
  GLOBAL_MUTEX = Mutex.new

  def mutex
    @mutex || GLOBAL_MUTEX.synchronize { @mutex ||= Mutex.new }
  end
end

That will definitely work on all Ruby versions and will avoid modifying initializer :D

Pushed the idea as a commit :D

[ivoanjo]

Hey @ixti 👋

I spoke with @delner and we're very happy to accept this version -- it's a great solution and contribution.

Indeed creating a mutex is so fast and specific that having a single shared lock for it does not seem like it will ever be a scalability limit.

I've left a couple of suggestions, but otherwise happy to merge this in :)

[ixti]

@ivoanjo Thank you! Applied the suggestions and cleaned-up commits :D

[ivoanjo]

Thanks again, merging away!

[delner]

@ixti thanks for the contribution! We really appreciate it. :)