[ruby-core:57974] [CommonRuby - Feature #8556] MutexedDelegator as a trivial way to make an object thread-safe

Issue #8556 has been updated by ko1 (Koichi Sasada).


Sorry for late.

----

Summary: I believe we need more experience before including this library as standard.

(1) Try gem first

Basically, libraries written in Ruby can be provided by a gem easilly.
We can prove the usefulness with real experiece by this approach.
In other words, we shouldn't provide any libraries without such proof.

(2) Misleading design

I'm afraid that this library introduce bugs under misunderstanding.

For example, people consider that this object is without worry about
synchronization, people may write the following program.

# In fact, *I* wrote this program first without any question!!

####
require 'delegate'
require 'monitor'

class SynchronizedDelegator < SimpleDelegator
  def initialize(*)
    super
    @monitor = Monitor.new
  end
  
  def method_missing(m, *args, &block)
    begin
      monitor = @monitor
      monitor.mon_enter
      super
    ensure
      monitor.mon_exit
    end
  end
end

sdel_ary = SynchronizedDelegator.new([0])

ary = [0]
m = Mutex.new

ts = (1..2).map{
  Thread.new{
    100_000.times{
      sdel_ary[0] += 1 # -> 1
      sdel_ary[0] -= 1 # -> 0

      m.synchronize{
        ary[0] += 1
        ary[0] -= 1
      }
    }
  }
}

ts.each{|t| t.join}
p sdel_ary #=> [40] # or something wrong result
p ary      #=> [0]
####

At first I see this result, I can't understand why.
Of course, this program is completely bad program.
It is completely my mistake.

But I think this design will lead such misunderstanding and bugs easily.

To avoid a such bug, I define the inc() and sub() method in Array.

####
class Array
  def inc; self[0] += 1; end
  def sub; self[0] -= 1; end
end

sdel_ary = SynchronizedDelegator.new([0])

ts = (1..2).map{
  Thread.new{
    100_000.times{
      sdel_ary.inc
      sdel_ary.sub
    }
  }
}

ts.each{|t| t.join}
p sdel_ary[0] #=> 200000
####

This works completely.

But a person who assumes sdel_ary is free from consideration about locking,
can write the following program:

####
class << sdel_ary
  def inc; self[0] += 1; end
  def sub; self[0] -= 1; end
end

ts = (1..2).map{
  Thread.new{
    100_000.times{
      sdel_ary.inc
      sdel_ary.sub
    }
  }
}

ts.each{|t| t.join}
p sdel_ary[0] #=> 229
####

This doesn't work correctly (different from the person expect).

I feel we can find other cases.

Maybe professional about concurrency program can avoid such silly bugs.
But if we introduce it as standard library, I'm afraid they are not.

(3) Lock based thraed programming

This is my opinion. So it is weak objection for this proposal.

I believe lock based thread programming introduced many bugs.
(Synchronized) Queue or more high-level structures should be used.

Or use Mutex (or monitor) explicitly for fing-grain locking.
It bothers programmers, so programmers use other approachs such as Queue (I hope).

Summary:
Mainly, my objection is based on (1) and (2).
Concurrency is a very difficult theme.
I feel 2.1 is too early to include this feature.
At least, we need more experience about this feature to introduce.

I'm not against that professionals use this libarary.

----------------------------------------
Feature #8556: MutexedDelegator as a trivial way to make an object thread-safe
https://bugs.ruby-lang.org/issues/8556#change-42550

Author: headius (Charles Nutter)
Status: Assigned
Priority: Normal
Assignee: ko1 (Koichi Sasada)
Category: 
Target version: Ruby 2.1.0


=begin
I propose adding (({MutexedDelegator})) as a simple way to wrap any object with a thread-safe wrapper, via existing delegation logic in ((%delegate.rb%)).

(({Delegator})) provides a way to pass method calls through to a wrapped object. (({SimpleDelegator})) is a trivial implementation that just holds the object in an instance variable. (({MutexedDelegator})) would extend (({SimpleDelegator})) and only override initialize and (({method_missing})) as follows:

  class MutexedDelegator < SimpleDelegator
    def initialize(*)
      super
      @mutex = Mutex.new
    end
    
    def method_missing(m, *args, &block)
      target, mutex = self.__getobj__, @mutex
      begin
        mutex.lock
        target.__send__(m, *args, &block)
      ensure
        mutex.unlock
      end
    end
  end

The only changes here are:

* (({Mutex#lock})) and (({unlock})) logic wrapping the send
* No (({respond_to?})) check; I'm not sure why it's there to begin with, since if we're in (({method_missing})) the (({super()})) call will fail just like a normal (({method_missing})) failure anyway
* No backtrace manipulation. This does not work on JRuby and Rubinius anyway, and in this case I feel that the delegator should not hide itself, since there's real behavior change happening.

This is a trivial addition to stdlib that would make it simple to synchronize all calls to a given object in the same way as the JDK's (({Collections.synchronizedSet}))/(({Map}))/(({List})) calls.
=end



-- 
http://bugs.ruby-lang.org/