-
1. Re: Valve on HAAspect
clebert.suconic Dec 17, 2006 12:04 AM (in response to 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) -
2. Re: Valve on HAAspect
clebert.suconic Dec 18, 2006 6:23 PM (in response to 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. -
3. Re: Valve on HAAspect
timfox Dec 19, 2006 7:02 AM (in response to clebert.suconic)"clebert.suconic@jboss.com" wrote:
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. -
4. Re: Valve on HAAspect
clebert.suconic Dec 19, 2006 10:43 AM (in response to 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 -
5. Re: Valve on HAAspect
timfox Dec 19, 2006 11:05 AM (in response to clebert.suconic)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. -
6. Re: Valve on HAAspect
clebert.suconic Dec 19, 2006 11:18 AM (in response to 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. -
7. Re: Valve on HAAspect
timfox Dec 19, 2006 11:19 AM (in response to clebert.suconic)This is BS, the whole point of a read lock, is multiple read locks can be acquired concurrently.
-
8. Re: Valve on HAAspect
clebert.suconic Dec 19, 2006 11:24 AM (in response to clebert.suconic)ok, my bad...
The Valve is using readWriteLocks anyway -
9. Re: Valve on HAAspect
timfox Dec 19, 2006 11:31 AM (in response to clebert.suconic)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. -
10. Re: Valve on HAAspect
clebert.suconic Dec 22, 2006 6:21 PM (in response to 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(); //} }
-
11. Re: Valve on HAAspect
timfox Dec 22, 2006 6:25 PM (in response to clebert.suconic)can you explain the deadlock in more detail?
-
12. Re: Valve on HAAspect
clebert.suconic Dec 22, 2006 6:59 PM (in response to 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. -
13. Re: Valve on HAAspect
timfox Dec 22, 2006 7:25 PM (in response to clebert.suconic)I'm not sure that just commenting out the synchronisation without understanding why the lock occurred is a good idea.
-
14. Re: Valve on HAAspect
clebert.suconic Dec 22, 2006 8:10 PM (in response to 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.