From: "ko1 (Koichi Sasada)" Date: 2013-10-22T23:55:48+09:00 Subject: [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/