1 2 Previous Next 17 Replies Latest reply on Dec 28, 2006 5:20 PM by Clebert Suconic

    Valve on HAAspect

    Clebert Suconic Master

      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,

        • 1. Re: Valve on HAAspect
          Clebert Suconic Master

          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 Master

            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
              Tim Fox Master

               

              "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 Master

                 

                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
                  Tim Fox Master

                  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 Master

                    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
                      Tim Fox Master

                      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 Master

                        ok, my bad...

                        The Valve is using readWriteLocks anyway

                        • 9. Re: Valve on HAAspect
                          Tim Fox Master

                          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 Master

                            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
                              Tim Fox Master

                              can you explain the deadlock in more detail?

                              • 12. Re: Valve on HAAspect
                                Clebert Suconic Master

                                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
                                  Tim Fox Master

                                  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 Master

                                    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.

                                    1 2 Previous Next