dry-rb / dry-cli

General purpose Command Line Interface (CLI) framework for Ruby
https://dry-rb.org/gems/dry-cli
MIT License
327 stars 41 forks source link

Remove `concurrent-ruby` as runtime dependency #90

Closed jodosha closed 4 years ago

jodosha commented 4 years ago

Enhancements

We recently discussed in chat about thread-safety, code loading, and if it was worth to keep concurrent-ruby around.

This proposal removes concurrent-ruby, and still keeps the thread-safety properties. It's a win-win situation, and dry-cli can have zero runtime gem dependencies.


One note: I removed concurrent-ruby gem from project.yml, IDK how to reflect this change in dry-cli.gemspec.

darigaaz commented 4 years ago

in my opinion cleaner approach will be as it does not pollute Foo class and its singleton_class also it incapsulates all behavour in module/modules and hides mutex from Foo

class SynchronizedAccess < ::Module
  def initialize
    __mutex__= nil

    super

    define_method(:initialize) do |*args|
      __mutex__ = Mutex.new

      super(*args)
    end

    # like `def self.synchronized` but as closure with __mutex__
    # defines instance methods on our instance module
    # calls original method wrapped in mutex
    singleton_class.define_method(:synchronized) do |method_name|
      define_method(method_name) do |*args|
        __mutex__.synchronize do
          super(*args)
        end
      end
    end
  end

  # to be included in class
  # to make class method `Foo.synchronized` available
  # as a closure around our instance of SynchronizedAccess
  class ClassMethods < ::Module
    def initialize(mod)
      __mod__ = mod

      super()

      define_method(:synchronized) do |method_name|
        __mod__.synchronized(method_name)
      end
    end
  end

  # @note dont like `base.singleton_class.define_method` as it defines method on Foo.singleton_class
  def prepended(base)
    base.extend ClassMethods.new(self)

    super
  end
end

class Foo
  prepend SynchronizedAccess.new

  synchronized def foo
    puts "it's safe here"
  end
end
jodosha commented 4 years ago

Folks, I've removed the synchronization around getters and setters.

I also tried to limit the amount of changes that you suggested. Specifically:

  1. I changed the examples using flatten to flatten(1), but not concat. If we see improvements needed here, please open another PR to address them.
  2. I din't went for the SynchronizedAccess module because the final implementation has only 5 places where Mutex is used, IMO it isn't worth the effort to DRY it. Verbosity drives simplicity, and it's fine until it's painful, but now we're really far from a painful situation.

Please let me know what do you think about it. Thanks all for the conversation! 🙌