[jboss-dev-forums] [Design of Messaging on JBoss (Messaging/JBoss)] - Re: Failover refactoring (Repost)

[clebert.suconic@jboss.com]

I had this conversation in private with Ovidiu, and I wanted to make a public post about this.

I'm kind of concerned about the fact we are using multiple valves (instead of a single instance per connection).

The way is implemented now, you have a Valve on every descendant of Connection (Sessions, Producers, Consumers and Browsers).

When we get a failure, we have to close every single valve before performing a failover, i.e. The lock is not atomic and you have multiple locks.

The problem is when you place a try..catch(IOException) on the ValveIntercetor to capture failure exceptions. In cases where you have multiple threads using the same Connection (multiple sessions/threads on the same Connection). As the lock should be aways atomic (close every valve for the failed Connection), you can have multiple Threads trying to close the whole hierarchy of valves.

Lets talk in examples:

- You have one Connection and 50 Threads/50 Sessions. Suppose all these 50 Threads are communicating at the same time.

- We kill the server, all of the 50 threads are going to catch an IOException at the same time.

- As all of the 50 threads are going to catch the exception at the same time... All of them are going to close the valve at the same time. Valve close is a loop on the hierarchy:


  |    // from HierarchicalStateSupport
  |    public void closeChildrensValves() throws Exception
  |    {
  |       if (children == null)
  |       {
  |          return;
  |       }
  | 
  |       for(Iterator i = children.iterator(); i.hasNext(); )
  |       {
  |          HierarchicalState s = (HierarchicalState)i.next();
  |          ((Valve)s.getDelegate()).closeValve();
  |       }
  |    }
  | 
  |    public void openChildrensValves() throws Exception
  |    {
  |       if (children == null)
  |       {
  |          return;
  |       }
  | 
  |       for(Iterator i = children.iterator(); i.hasNext(); )
  |       {
  |          HierarchicalState s = (HierarchicalState)i.next();
  |          ((Valve)s.getDelegate()).openValve();
  |       }
  |    }
  | 

I belive we should use a single Valve instance, the way it was done before.
The only reason for this refactoring was the fact I was using a single instance but using AOP API to install the Valve on the right place with the proper instantiation. This could be refactored in another way such as delegating the Valve to an external object on the ValveAspect (which was going to be renamed to FailureInterceptor).

I'm playing with another Branch and I have already refactored it to use a single instance of Valve, and have installed the try...catch(IOException) in place. I'm only using standard XML config.

View the original post : http://www.jboss.com/index.html?module=bb&op=viewtopic&p=3998012#3998012

Reply to the post : http://www.jboss.com/index.html?module=bb&op=posting&mode=reply&p=3998012