Review My Code! (Pooling)

[Sam Smoot]

Anyone with threading experience, please take a peek and critique:

http://github.com/sam/extlib/tree/master/lib/extlib/pooling.rb#L36

The double-lock-verify steps concern me the most.

Also wondering if Mutex#synchronize is causing issues. I was assuming
it would verify the Mutex#locked? in-thread, but it doesn't appear to
since nested locks in the same thread stall. So I may switch to the
non-block form using #lock and #unlock so methods can be protected,
but transparently used if the lock is already obtained in the same
thread.

Anyone that feels like contributing feel free. And don't be shy. If
you think something's stupid in the above code, call me out on it. :-)

[Wilson Bilkovich]

On Jun 12, 2:14 pm, Sam Smoot <ssm...@gmail.com> wrote:
> Anyone with threading experience, please take a peek and critique:
>
> http://github.com/sam/extlib/tree/master/lib/extlib/pooling.rb#L36
>
> The double-lock-verify steps concern me the most.
>

Can you explain why there are two locks?
In particular, I am looking at:

lock.synchronize do
pools.each do |pool|
if pool.expired?
pool.lock.synchronize do
if pool.size == 0
pool.dispose
end
end
end
end
end

The outer lock is acquired.. each pool yielded, then that pool's lock
is acquired, and the pool is disposed if necessary.

Given that you have to acquire the outer lock before even looping over
the pools, I don't see why the inner lock is necessary.
Do the Pool instances need other operations to run on them while they
are being checked for disposal?

[Wilson Bilkovich]

On Jun 12, 2:14 pm, Sam Smoot <ssm...@gmail.com> wrote:

> Anyone with threading experience, please take a peek and critique:
>
> http://github.com/sam/extlib/tree/master/lib/extlib/pooling.rb#L36
>

Oh, and I really think you should avoid that style of 'rescue'

rescue => ex
... do whatever ..
raise ex
end

is better in my opinion, because the behavior is easy to understand
even when nested rescues and ensures are in play.

[Sam Smoot]

Will do, thanks. It was just for debugging, I'll remove it actually.

[Mark Bates]

I'm not sure, but I'm guessing it's being done for the same reason you
do this when creating a Singleton in Java:

public static KeyManager getInstance()
{
if (instance == null)
{
synchronized (KeyManager.class)
{
if (instance == null)
{
instance = new KeyManager();
}
}
}

return instance;
}

That solves the case of two people hitting the code at the same time.
They both hit and get through the first instance == null check. Then
only one of the gets through the synchronized block while the other
waits with bated breath to get in. Once the first the guy creates the
instance and leaves the second guy comes in and gets turned away by
the second instance == null check.

That's just a guess though, I haven't had a chance to deep dive into
the code.

-------------------------------------------------------------------------------------------------
Mark Bates
ma...@mackframework.com
http://www.mackframework.com
http://api.mackframework.com/
http://github.com/markbates/mack

[Sam Smoot]

On Jun 12, 3:42 pm, Mark Bates <m...@mackframework.com> wrote:
> I'm not sure, but I'm guessing it's being done for the same reason you  
> do this when creating a Singleton in Java:
>
>      public static KeyManager getInstance()
>      {
>          if (instance == null)
>          {
>              synchronized (KeyManager.class)
>              {
>                  if (instance == null)
>                  {
>                      instance = new KeyManager();
>                  }
>              }
>          }
>
>          return instance;
>      }
>
> That solves the case of two people hitting the code at the same time.  
> They both hit and get through the first instance == null check. Then  
> only one of the gets through the synchronized block while the other  
> waits with bated breath to get in. Once the first the guy creates the  
> instance and leaves the second guy comes in and gets turned away by  
> the second instance == null check.
>
> That's just a guess though, I haven't had a chance to deep dive into  
> the code.
>
> --------------------------------------------------------------------------- ----------------------
> Mark Bates

> m...@mackframework.comhttp://www.mackframework.comhttp://api.mackframework.com/http://github.com/markbates/mack

>
> On Jun 12, 2008, at 2:58 PM, Wilson Bilkovich wrote:
>
>
>
> > On Jun 12, 2:14 pm, Sam Smoot <ssm...@gmail.com> wrote:
> >> Anyone with threading experience, please take a peek and critique:
>
> >>http://github.com/sam/extlib/tree/master/lib/extlib/pooling.rb#L36
>
> >> The double-lock-verify steps concern me the most.
>
> > Can you explain why there are two locks?
> > In particular, I am looking at:
>
> > lock.synchronize do
> >  pools.each do |pool|
> >    if pool.expired?
> >      pool.lock.synchronize do
> >        if pool.size == 0
> >          pool.dispose
> >        end
> >      end
> >    end
> >  end
> > end
>
> > The outer lock is acquired.. each pool yielded, then that pool's lock
> > is acquired, and the pool is disposed if necessary.
>
> > Given that you have to acquire the outer lock before even looping over
> > the pools, I don't see why the inner lock is necessary.
> > Do the Pool instances need other operations to run on them while they
> > are being checked for disposal?

Right, that's the general idea, except it's been a long time since I
had to deal with proper threads. :-) ie: http://www.yoda.arachsys.com/csharp/singleton.html

So it looks similar, but I was mixing concerns so it's deceptive.

The deal here is that the outer synchronize is simply to protect the
@pools Set from being modified while iterated since I was too lazy to
double-check if Ruby's Set was thread-safe.

The inner synchronize is to block any pooled-object aquires/releases
while the Pool is scavenged and potentially disposed of.

The general idea is that each type of pooled object gets a pool for
each possible set of arguments an object could be initialized with. So
3 different connection-strings for example would produce 3 difference
pools.

Then there's a single scavenger thread that blocks the whole process-
wide object-pooling effort, no matter the type of object, and it
checks each Pool, forcing the Pool to consider if any of it's
available but un-aquired objects are candidates for disposal. Once
that's done, it checks to see if the Pool as a whole has any remaining
objects. If there are no currently reserved/leased objects, then the
entire Pool is disposed of, to ensure that the scavenging process
remains cheap for long-lived processes over time and doesn't
infinitely grow.

So that's the goal. It's the implementation that concerns me. I'm not
100% confident that I don't have a potential dead-lock waiting in
hiding here. Which is why I seek your fellow's sage advice.

Thanks, -Sam

[sambo99]

Sam,

I think

@scavenger || begin
@scavenger = Thread.new do

Is problem code.

If called concurrently you can get a situation where you have 2
scavenger threads (or more).

you need a lock and a double check before you assign the new thread.

On Jun 13, 6:53 am, Sam Smoot <ssm...@gmail.com> wrote:
> On Jun 12, 3:42 pm, Mark Bates <m...@mackframework.com> wrote:
>
>
>
> > I'm not sure, but I'm guessing it's being done for the same reason you  
> > do this when creating a Singleton in Java:
>
> >      public static KeyManager getInstance()
> >      {
> >          if (instance == null)
> >          {
> >              synchronized (KeyManager.class)
> >              {
> >                  if (instance == null)
> >                  {
> >                      instance = new KeyManager();
> >                  }
> >              }
> >          }
>
> >          return instance;
> >      }
>
> > That solves the case of two people hitting the code at the same time.  
> > They both hit and get through the first instance == null check. Then  
> > only one of the gets through the synchronized block while the other  
> > waits with bated breath to get in. Once the first the guy creates the  
> > instance and leaves the second guy comes in and gets turned away by  
> > the second instance == null check.
>
> > That's just a guess though, I haven't had a chance to deep dive into  
> > the code.
>
> > --------------------------------------------------------------------------- ----------------------
> > Mark Bates

> > m...@mackframework.comhttp://www.mackframework.comhttp://api.mackframework.com/http://githu...

[go94022]

Note that this appears to be an example of a "double-checked locking"
pattern.
http://en.wikipedia.org/wiki/Double-checked_locking
http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html

Unless Ruby can guarantee atomic checking (or equivalent memroy
barrier semantics), this will lead to rare, obscure failures under
load.

> > Thanks, -Sam- Hide quoted text -
>
> - Show quoted text -