Synchronized attr_accessor

[Nasir Khan]

I have a fairly repetitive use case of having to define attributes and
have them lock protected. I was thinking of defining a attr_accessor
kind of directive but one that creates a lock protected attribute -

class Module
def attr_sync_accessor(*symbols)
symbols.each do |symbol|
str = <<-EOF
def #{symbol}()
if @#{symbol}
@#{symbol}.synchronize { return @#{symbol} }
else
nil
end
end
EOF
module_eval(str)
str = <<-EOF
def #{symbol}=(val)
if @#{symbol}
@#{symbol}.synchronize { @#{symbol} = val }
else
@#{symbol} = val
@#{symbol}.extend(MonitorMixin)
end
end
EOF
module_eval(str)
end
end
end

#--------------

Now to use it you need to require monitor so

require 'monitor'

class A
attr_sync_accessor :hello
def get
hello
end
def set(val)
self.hello = val
end
end

irb(main):190:0* d = A.new
=> #<D:0x27f77d4>
irb(main):191:0> d.set "h"
=> "h"
irb(main):192:0> d.get
=> "h"

This works but obviously has some flaws, like for example first time
creation of attribute is unprotected as is the nil check, also it does
not address scope [private, protected etc.].

I am looking for suggestions on improvements to this if anyone else
also finds it useful or if it is solved by someone in some other way
and I have overlooked something.

TIA
- Nasir

 

[MenTaLguY]

This works but obviously has some flaws, like for example first time
creation of attribute is unprotected as is the nil check, also it does
not address scope [private, protected etc.].

Could you describe are you trying to accomplish using these attributes? There is usually a better way to communicate between threads than setting/getting attributes on an object.

-mental

 

[Nasir Khan]

This is about having a instance variable always accessed under a lock,
if object state (instance variables) are accessed by many threads at
the same time
for eg. a hash accessed by several threads for getting values but some
operation also adding deleting from the hash. You would want a
synchronized access to your hash in this case...

So this could save some repetitive lines of code, that is all.

- nasir

This works but obviously has some flaws, like for example first time
creation of attribute is unprotected as is the nil check, also it does
not address scope [private, protected etc.].

Could you describe are you trying to accomplish using these attributes? There is usually a better way to communicate between threads than setting/getting attributes on an object.

-mental

 

[MenTaLguY]

This is about having a instance variable always accessed under a lock,
if object state (instance variables) are accessed by many threads at
the same time for eg. a hash accessed by several threads for getting values but some
operation also adding deleting from the hash. You would want a
synchronized access to your hash in this case...

Just because access to an instance variable is synchronized does not mean that access to the object it references is synchronized. Retrieving a hash object via a synchronized accessor does not make it safe against concurrent modification.

There are approaches which will work, but to know which ones to suggest, I will need to know more about the "bigger picture" that this code is part of.

-mental

 

[Nasir Khan]

There is no big picture.
The problem is simple - "create accessors for an instance variable
that access the variable under a lock".
Just as problem statement for "attr_accessor" could be - "create
accessors for instance variables"

- Nasir

PS: I understand that a synchronized access to hash is not the same as
synchronized access to hash values, but this problem is *not* about
it.

 

[Robert Dober]

There is no big picture.
The problem is simple - "create accessors for an instance variable
that access the variable under a lock".
Just as problem statement for "attr_accessor" could be - "create
accessors for instance variables"

Hmm there is no such thing as a lock on an ivar.
By declaring a synchronized accessor there is just no way to know
anything about the behavior of the accessed object, it might be
threadsafe or immutable, therefore it does not make much sense to me
and if I have understood your code correctly neither to Ruby. Imagine
someone using your synchronized accessor to change the ivar to a
simple integer, your code would fail at the next access attempt.

Cheers
Robert

 

[MenTaLguY]

There is no big picture.

Since you had asked for feedback on anything you'd missed, I was trying to find out if was some mitigating factor in the specific way your program worked (the one for which you originally wrote this code), before telling you that it won't work.

But -- unless the objects the attributes were set to were never modified, even if they didn't have the thread safety problems you already noted, the accessors generated still couldn't ensure thread safety. If thread 1 calls obj.some_accessor.foo, and thread 2 calls obj.some_accessor.bar (where #bar is some mutating method), some_accessor being synchronized simply _will not_ protect you.

-mental

 

[Nasir Khan]

Yeah I see what you guys mean...I was a little delusional. Now I
realize that I was looking for basically something equivalent to
Java's -
Collections.synchronizedMap(new HashMap(...))
...
Thanks

 

[Nasir Khan]

Actually facets/more/synchash.rb does what I was looking for hash.

Thanks

 

[Caleb Clausen]

define_method(meth) do |*args|
@__ms_lock = Mutex.new unless @__ms_lock
@__ms_lock.synchronize do
self.send("__nonsync_#{meth}",*args)
end
end

This still isn't completely thread-safe, I'm afraid. If the lock
doesn't exist yet and two threads both call some synchronized method
on the same object, you've got a race condition. I'd suggest creating
the lock upfront, before the synchronization wrappers are defined....
unfortunately, that makes implementing this idea a bit more
complicated.

 

[Caleb Clausen]

Thanks for the feedback. Here is a refinement -

I think there's still a race condition... you need to change this:

@@__ms_c_lock.synchronize { @__ms_lock = Mutex.new unless @__ms_lock
} unless @__ms_lock

To this:

@@__ms_c_lock.synchronize { @__ms_lock = Mutex.new unless @__ms_lock
unless @__ms_lock }

Which is somewhat uglier, since now the class level mutex has to be
checked on every synchronized method call.

Now, what I had in mind was a little more like this (INCOMPLETELY TESTED):

module MethodSynchronizer

def MethodSynchronizer.included(into)
into.sync_methods.each do |m|
MethodSynchronizer.wrap_method(into, m)
end
end

def MethodSynchronizer.wrap_method(klass, meth)
klass.class_eval do
alias_method "__nonsync_#{meth}", "#{meth}"
require 'thread'
define_methodinitialize_synchronizer) do
@__ms_lock=Mutex.new
return self
end
define_method(meth) do |*args|
@__ms_lock.synchronize do
self.send("__nonsync_#{meth}",*args)
end
end
end
end
end


And now you have to make sure that #initialize_synchronizer is called
on the object before any of the synchronized methods.

(What I really was thinking of was hacking into the regular
#initialize to get it to do the extra initialization automatically...
That can be hairy; among other things, you have to get it into the
#initialize of the class being mixed-in to, rather than the module's
#initialize. It's not impossible, but it requires more code than I
want to write right now.)

 

[Caleb Clausen]

What is the race condition you are talking about in the first snippet above?
And why did you change it to have two identical unless's one after the other
in the second snippet?
Note the unless is checking for @__ms_lock in *both* the unless's.

Uh-oh, I misunderstood your code. I didn't notice that you had unless
inside the lock too. I apologize.

I wasn't even aware of this issue MenTaLguY is talking about. I would
have thought that your second version worked, (as indeed it does in
1.8, although this behavior isn't advertized) although I still prefer
my explicit initialization beforehand for something like this.....
however, is that even a valid solution? After reading MenTaLguY's link
I'm not so sure...