Valve on HAAspect

[clebert.suconic]

I have implemented a Valve (class org.jboss.jms.util.Valve) which will prevent two threads to enter in the same block, where the second thread will just ignore the routine.

Valve.open will return true only once... and it will wait until the first thread to execute open calls a close.

I have implemented this routine into HAAspect::handleConnectionException, however I don't know what to do if two subsequent failures happen, but not at the same time.

If two failures happen at the same time the method I added is ok, but if a second failure happens seconds after the method is done, then the method will assume another failover is happening. On that case hpping logic should take care of that.

Ok.. I guess I'm drunk and maybe my explanation is too confused. Let me explain through an example:

Example I:
- Thread A and B, both got an IOException at the same time from ConnectionListener.
On this case the valve will ignore one of the exceptions and will perform the client failover in only one exception

Example II:
- Thread A get an exception...
- Failover happens
- Thread B get an exception after failover on client is done
- Failover will be called again, and I'm expecint hopping will be smart enough to ignore the second failover event.


if you could please take a look at the code (Ovidiu, Tim, contributors) to verify if the implementation is correct. Maybe you thought about something I didn't think.


Cheers,

[clebert.suconic]

I have just committed ValveAspect, as an extension of HAAspect.

I want to have a design session with developers about this. if something
is not right I can refactor it later.


I could change this to use the aspect's XML, but from my point of view
it would complicate another side of things (like how to get the
ConnectinDelegate from any Delegate, and how to get the proper Valve in
place). Right now I'm adding the Aspect to the stack using the API. This is to control the instantiation of ValveAspect, and also we will only install the aspect on clustered connection factories.

We could play with two stacks, but the instantiation needs also to be taken in consideration. If we use plain pointcuts it will require us to aways navigate to ConnectionState/ConnectionDelegate from any given Delegate what would probably make the aspect use some ifdefs. (we can discuss this over a design session)

As you can see I have also created an abstract type Valve, which
encapsulates synchronized blocks and notifies. It's a very simple class
but I liked the way I did, encapsulating the logic over there.
(org.jboss.jms.util.Valve). It's a functionality so basic that I
wondered if anyone else ever implemented such thing (probably yes).

Another point is about ValveAspect extending HAAspect. I could move
everything to HAAspect but it looked too messy when I had it only in one
class. First because HAAspect exists per ConnectionFactory, while
ValveAspect exists per Connection, and the use cases are a little bit
different. ValveAspect will call failover in a connection, but it will
block simultaneous calls.

I have also moved the ConnectionListener into ValveAspect as I don't
want simultaneous failover calls captured by try..catch or Remoting
Lease.

I still have a failure on ValveTest, but it's not caused by the Valve
itself. The multi-thread has made the case a little bit worse, so we
will have to investigate this independently of the valve development.
Something about the Message is not on the channel (Paging support)

[clebert.suconic]

I have locally changed ValveAspect to use readWriteLocks... But I think using org.jboss.messaging.util.Valve would be a better mechanism.

a parenthesis before starting (Valve is the same as creating a property failoverRunning on the Aspect, and if failoverRunning is true we will wait on notify until failover is finished. I just encapsulated such logic in a class instead of spread Objects and notifies on ValveAspect, so please do not compare it with java.util.concurrent or any other package like that)


Using readWriteLock will add synchronization into ValveAspect::invoke. Even readWriteLock requires synchronization while the use of Valve would minimize it.

As a comparisson I have written ValidateValveLogicValveTest and ValidateValveLogicReadWriteTest which are going to play with counters and doing the exact same locking mechanism needed on ValveAspect.

ValidateValveLogicValveTest could perform more or less 4 times the number of loops performed by ValidateValveLogicReadWriteTest. This is because the object.wait() added readLock().acquire().

on a thread dump, I could see this from readLock().acquire():

"Thread-999" prio=1 tid=0x087bacd8 nid=0x37a0 waiting for monitor entry [0x4e456000..0x4e456228]
 at EDU.oswego.cs.dl.util.concurrent.WriterPreferenceReadWriteLock$ReaderLock.acquire(WriterPreferenceReadWriteLock.java:159)
 - waiting to lock <0x746d4428> (a EDU.oswego.cs.dl.util.concurrent.WriterPreferenceReadWriteLock$ReaderLock)
 at org.jboss.test.messaging.util.ValidateValveLogicReadWriteTest$ThreadRead.run(ValidateValveLogicReadWriteTest.java:198)

"Thread-998" prio=1 tid=0x087b9fd0 nid=0x379f waiting for monitor entry [0x4e4d7000..0x4e4d7228]
 at EDU.oswego.cs.dl.util.concurrent.WriterPreferenceReadWriteLock$ReaderLock.acquire(WriterPreferenceReadWriteLock.java:159)
 - waiting to lock <0x746d4428> (a EDU.oswego.cs.dl.util.concurrent.WriterPreferenceReadWriteLock$ReaderLock)
 at org.jboss.test.messaging.util.ValidateValveLogicReadWriteTest$ThreadRead.run(ValidateValveLogicReadWriteTest.java:198)

"Thread-997" prio=1 tid=0x087b92c8 nid=0x379e waiting for monitor entry [0x4e558000..0x4e558228]
 at EDU.oswego.cs.dl.util.concurrent.WriterPreferenceReadWriteLock$ReaderLock.acquire(WriterPreferenceReadWriteLock.java:159)
 - waiting to lock <0x746d4428> (a EDU.oswego.cs.dl.util.concurrent.WriterPreferenceReadWriteLock$ReaderLock)
 at org.jboss.test.messaging.util.ValidateValveLogicReadWriteTest$ThreadRead.run(ValidateValveLogicReadWriteTest.java:198)

Of course I'm not trying to reinvent the wheel here, but I just think we have an useCase where we don't want to synchronize readLocks on the invocation, as we want to avoid new calls to be performed while client failover is being executed, not to add synchronization on such basic feature.

I'm assuming here it is a common practice to have multiple sessions associated with a single connection. The synchronization here will be per Connection. If customers or public benchmarks are using muliple connections, then this shouldn't be a problem.

[timfox]

I have locally changed ValveAspect to use readWriteLocks... But I think using org.jboss.messaging.util.Valve would be a better mechanism.

a parenthesis before starting (Valve is the same as creating a property failoverRunning on the Aspect, and if failoverRunning is true we will wait on notify until failover is finished. I just encapsulated such logic in a class instead of spread Objects and notifies on ValveAspect, so please do not compare it with java.util.concurrent or any other package like that)


Using readWriteLock will add synchronization into ValveAspect::invoke. Even readWriteLock requires synchronization while the use of Valve would minimize it.

How does the use of Valve minimise it? Valve still needs to be synchronised.

[clebert.suconic]

How does the use of Valve minimise it? Valve still needs to be synchronised.

It synchronizes access to the attribute isRunning but it releases the attribute very fast, while lock will require synchronizationo for the entire call.

With Locks:

handleInvocation:
.... lock.
.... performAspect
.... release lock

with Valves:

handleInvocation
.... lock
.... access atributes
.... verify if the valve is free
.... release locks
..... perform Aspect

[timfox]

That is simply not true.

Acquiring a reader or writer lock only synchronizes for the duration of the acquire call.

If you don't believe me look at the source.

[clebert.suconic]

The lock I mentioned is what is happening inside acquire. look at the thread dump I posted before. (PostPosted: Mon Dec 18, 2006 18:23 PM)

You don't have a lock, but you will have a wait() on another release(), what will mean the method virtually locking until a readLock is released.

[timfox]

This is BS, the whole point of a read lock, is multiple read locks can be acquired concurrently.

[clebert.suconic]

ok, my bad...

The Valve is using readWriteLocks anyway

[timfox]

I would very strongly recommend getting familiar with Doug Lea's concurrent classes:

For 1.4: http://g.oswego.edu/dl/classes/EDU/oswego/cs/dl/util/concurrent/intro.html

For 1.5: Look in java.util.concurrent.

In my experience it is very unusual to have to write your own synchronisation primitives since most things are available there.

[clebert.suconic]

I have reactivated ValveAspect on my local box, and I had a dead lock with MessageCallbackHandler::copyState.

Another thread was holding a lock to the MessageCallbackHandler while clientFailover was trying to call copyState whith a waiting to lock.

I had to remove the lock, and I will probably commit this:

 public void copyState(MessageCallbackHandler newHandler)
 {
 //synchronized (mainLock)
 //{
 this.consumerID = newHandler.consumerID;

 this.consumerDelegate = newHandler.consumerDelegate;

 this.sessionDelegate = newHandler.sessionDelegate;

 this.serverSending = false;

 this.buffer.clear();
 //}
 }

[timfox]

can you explain the deadlock in more detail?

[clebert.suconic]

I don't have the thread dump any more... I will replicate this again, and I will post the traces as soon as I have it.

MessageCallbackHandler was holding a lock while failover was being called.

Then when client failover happened, MessageCallbackHandler was holding the valve (don't know where? Don't know if it's valid), while the valve was trying to perform failover in a different thread. Result was a dead lock.

[timfox]

I'm not sure that just commenting out the synchronisation without understanding why the lock occurred is a good idea.

[clebert.suconic]

When I got the lock I though it was obvious one...

But we certainly have a race condition on this.. .I will try to replicate.