5 Replies Latest reply on Apr 22, 2004 3:40 PM by Juha Lindfors

    org.jboss.ms.server: Dispatcher and Invocation impl

    Alexey Loubyansky Master

      There are problems with the current implementation. The dispatcher must not own args but the invocation should. The problem is that dispatcher is not thread-safe and concurrent threads override each other's parameters.

      The consequences are:
      - different threads associated with the same transaction;
      - faulty re-entered method calls;
      - concurrent method calls on stateful session beans;
      - unexpected UndeclaredThrowableException's invoking MBeans;
      - many many other very weird behaviours.

      This is the diff of the fix. Is it ok to commit or do you have any remarks?

      M mx/server/AttributeDispatcher.java
      [aloubyansky@dedicated74 server]$ cvs diff AttributeDispatcher.java
      loubyansky@cvs.sourceforge.net's password:
      Index: AttributeDispatcher.java
      ===================================================================
      RCS file: /cvsroot/jboss/jmx/src/main/org/jboss/mx/server/AttributeDispatcher.java,v
      retrieving revision 1.1.2.1
      diff -r1.1.2.1 AttributeDispatcher.java
      30c30
      < public Object dispatch() throws InvocationException
      ---
      > public Object dispatch(Object[] args) throws InvocationException
      
      
      M mx/server/Dispatcher.java
      [aloubyansky@dedicated74 server]$ cvs diff Dispatcher.java
      loubyansky@cvs.sourceforge.net's password:
      Index: Dispatcher.java
      ===================================================================
      RCS file: /cvsroot/jboss/jmx/src/main/org/jboss/mx/server/Dispatcher.java,v
      retrieving revision 1.4.2.1
      diff -r1.4.2.1 Dispatcher.java
      18,23c18
      <
      < public void setArgs(Object[] args);
      <
      < public Object[] getArgs();
      <
      < public Object dispatch() throws InvocationException;
      ---
      > public Object dispatch(Object[] args) throws InvocationException;
      
      M mx/server/Invocation.java
      [aloubyansky@dedicated74 server]$ cvs diff Invocation.java
      loubyansky@cvs.sourceforge.net's password:
      Index: Invocation.java
      ===================================================================
      RCS file: /cvsroot/jboss/jmx/src/main/org/jboss/mx/server/Invocation.java,v
      retrieving revision 1.5.2.1
      diff -r1.5.2.1 Invocation.java
      23c23,24
      <
      ---
      > private Object[] args;
      >
      38a40,49
      >
      > public void setArgs(Object[] args)
      > {
      > this.args = args;
      > }
      >
      > public Object[] getArgs()
      > {
      > return args;
      > }
      41a53,57
      > return dispatch(args);
      > }
      >
      > public Object dispatch(Object[] args) throws InvocationException
      > {
      45c61
      < return dispatcher.dispatch();
      ---
      > return dispatcher.dispatch(args);
      
      M mx/server/InvocationContext.java
      [aloubyansky@dedicated74 server]$ cvs diff InvocationContext.java
      loubyansky@cvs.sourceforge.net's password:
      Index: InvocationContext.java
      ===================================================================
      RCS file: /cvsroot/jboss/jmx/src/main/org/jboss/mx/server/InvocationContext.java,v
      retrieving revision 1.7.2.2
      diff -r1.7.2.2 InvocationContext.java
      120,124d119
      < public void setArgs(Object[] args)
      < {
      < this.dispatcher.setArgs(args);
      < }
      <
      128a124
      >
      134,138d129
      < public Object[] getArgs()
      < {
      < return this.dispatcher.getArgs();
      < }
      <
      199,211c190
      < private Object[] args = null;
      <
      < public Object[] getArgs()
      < {
      < return args;
      < }
      <
      < public void setArgs(Object[] args)
      < {
      < this.args = args;
      < }
      <
      < public Object dispatch() throws InvocationException
      ---
      > public Object dispatch(Object[] args) throws InvocationException
      
      M mx/server/ReflectedDispatcher.java
      [aloubyansky@dedicated74 server]$ cvs diff ReflectedDispatcher.java
      loubyansky@cvs.sourceforge.net's password:
      Index: ReflectedDispatcher.java
      ===================================================================
      RCS file: /cvsroot/jboss/jmx/src/main/org/jboss/mx/server/ReflectedDispatcher.java,v
      retrieving revision 1.4.2.3
      diff -r1.4.2.3 ReflectedDispatcher.java
      38d37
      < protected Object[] args = null;
      54,64c53
      < public void setArgs(Object[] args)
      < {
      < this.args = args;
      < }
      <
      < public Object[] getArgs()
      < {
      < return args;
      < }
      <
      < public Object dispatch() throws InvocationException
      ---
      > public Object dispatch(Object[] args) throws InvocationException
      


        • 1. Unification of invocation and interceptors
          Adrian Brock Master

          As a start to the discussion of how to unify the invocation and interceptor layers used in the aop, jmx, remoting and server frameworks, here are the some base classes derived from the current aop layer. We need to come to a consenus on what the comment layer should look like. This layer should be in the jmx module.


          /** Pretty basic common interface
          */
          public interface Interceptor
          {
           public String getName();
           public Object invoke(Invocation invocation) throws Throwable;
          }
          


          /** An encapsulation of the response: (value or exception) + attributes
          */
          public class InvocationResponse
           implements java.io.Externalizable
          {
           // Constants -----------------------------------------------------
           static final long serialVersionUID = -718723094688127810L;
          
           // The Map of methods used by this Invocation
           protected HashMap contextInfo = null;
           protected Object response = null;
           protected Throwable responseThrowable;
          
           public HashMap getContextInfo()
           {
           return contextInfo;
           }
           public void setContextInfo(HashMap contextInfo)
           {
           this.contextInfo = contextInfo;
           }
          
          
           // Constructors --------------------------------------------------
           public InvocationResponse()
           {
           }
           public InvocationResponse(Object obj)
           {
           this.response = obj;
           }
          
           public Object getResponse()
           {
           return response;
           }
           public void setResponse(Object obj)
           {
           this.response = obj;
           }
          
           public Throwable getResponseThrowable
           {
           return responseThrowable;
           }
           public void setResponseThrowable(Throwable cause)
           {
           this.responseThrowable = cause;
           }
          
           public void addAttachment(Object key, Object val)
           {
           if (contextInfo == null) contextInfo = new HashMap(1);
           contextInfo.put(key, val);
           }
          
           public Object getAttachment(Object key)
           {
           if (contextInfo == null) return null;
           return contextInfo.get(key);
           }
          
           // Externalizable implementation ---------------------------------
           public void writeExternal(java.io.ObjectOutput out)
           throws IOException
           {
           ...
           }
          
           public void readExternal(java.io.ObjectInput in)
           throws IOException, ClassNotFoundException
           {
           ...
           }
          }
          


          /** This class varies significantly... Here is a variation from
          the aop layer.
          */
          public class Invocation implements java.io.Serializable
          {
           protected InvocationType type;
           protected MetaData metadata = null;
           protected transient int currentInterceptor = 0;
           protected transient Interceptor[] interceptors = null;
           protected transient Object targetObject = null;
           protected transient InvocationResponse response;
          
           public Invocation(InvocationType type, Interceptor[] interceptors)
           {
           this(type, interceptors, null);
           }
          
           public Invocation(InvocationType type, Interceptor[] interceptors, MetaData meta)
           {
           this.interceptors = interceptors;
           this.type = type;
           this.metadata = meta;
           }
          
           public Invocation(Invocation invocation)
           {
           this.type = invocation.getType();
           this.interceptors = invocation.getInterceptors();
           targetObject = invocation.targetObject;
           }
          
           public final InvocationType getType()
           {
           return type;
           }
          
          
           public Object getTarget()
           {
           return targetObject;
           }
           public void setTarget(Object target)
           {
           this.targetObject = target;
           }
          
           public final InvocationResponse getResponse()
           {
           if( response == null )
           response = new InvocationResponse();
           return response;
           }
          
          
           public final void addResponseAttachment(Object key, Object val)
           {
           getResponse().addAttachment(key, val);
           }
           public final Object getResponseAttachment(Object key)
           {
           Object val = getResponse().getAttachment(key);
           return val;
           }
          
          
           /**
           * Return all the contextual data attached to this invocation
           */
           public final MetaData getMetaData()
           {
           if (metadata == null) metadata = new SimpleMetaData();
           return metadata;
           }
           /**
           * Set all the contextual data attached to this invocation
           */
           public final void setMetaData(MetaData data)
           {
           this.metadata = data;
           }
          
           /**
           * Invoke on the next interceptor in the chain. If this is already
           * the end of the chain, reflection will call the constructor, field, or
           * method you are invoking on.
           */
           public final Object invokeNext() throws Throwable
           {
           try
           {
           return interceptors[currentInterceptor++].invoke(this);
           }
           finally
           {
           // so that interceptors like clustering can reinvoke down the chain
           currentInterceptor--;
           }
           }
          
           /** Is this useful?
           *
           */
           public final Object invokeNext(org.jboss.aop.advice.Interceptor[] newInterceptors) throws Throwable
           {
           // Save the old stack position
           org.jboss.aop.advice.Interceptor[] oldInterceptors = interceptors;
           int oldCurrentInterceptor = currentInterceptor;
          
           // Start the new stack
           interceptors = newInterceptors;
           currentInterceptor = 0;
          
           // Invoke the new stack
           try
           {
           return invokeNext();
           }
           finally
           {
           // Restore the old stack
           interceptors = oldInterceptors;
           currentInterceptor = oldCurrentInterceptor;
           }
           }
          
           public final Interceptor[] getInterceptors()
           {
           return interceptors;
           }
          
          
           public final void setInterceptors(Interceptor[] interceptors)
           {
           this.interceptors = interceptors;
           }
          
          
           /**
           * This method resolves metadata based on the context of the invocation.
           * It iterates through its list of MetaDataResolvers to find out the
           * value of the metadata desired. Can't these be embedded in the metadata?
           *
           */
           public Object getMetaData(Object group, Object attr)
           {
           Object val = null;
           if (this.metadata != null)
           {
           val = this.metadata.resolve(this, group, attr);
           if (val != null) return val;
           }
          
           return null;
           }
          }
          


          /** Why should MetaData know how to resolve which meta data is needed for a
           a given invocation?
           */
          public class SimpleMetaData implements MetaDataResolver, java.io.Serializable
          {
           protected HashMap metaData = new HashMap();
          
           public static class MetaDataValue implements java.io.Serializable
           {
          
           public final PayloadKey type; // One of "TRANSIENT", "AS_IS", "MARSHALLED"
           public final Object value;
          
           public MetaDataValue(PayloadKey type, Object value)
           {
           this.type = type;
           this.value = value;
           }
          
           public Object get()
           throws IOException, ClassNotFoundException
           {
           if (value instanceof MarshalledValue)
           {
           return ((MarshalledValue)value).get();
           }
           return value;
           }
          
           }
          
           public synchronized HashSet groups()
           {
           return new HashSet(metaData.keySet());
           }
          
           public synchronized HashMap group(String name)
           {
           HashMap map = (HashMap)metaData.get(name);
           if (map == null) return null;
           return (HashMap)map.clone();
           }
          
           public synchronized boolean hasGroup(String name)
           {
           return metaData.get(name) != null;
           }
          
          
           public void addMetaData(Object group, Object attr, Object value)
           {
           addMetaData(group, attr, value, PayloadKey.MARSHALLED);
           }
          
           public synchronized void addMetaData(Object group, Object attr, Object value, PayloadKey type)
           {
           HashMap groupData = (HashMap)metaData.get(group);
           if (groupData == null)
           {
           groupData = new HashMap();
           metaData.put(group, groupData);
           }
           MetaDataValue val = new MetaDataValue(type, value);
           groupData.put(attr, val);
           }
          
           public synchronized Object getMetaData(Object group, Object attr)
           {
           try
           {
           HashMap groupData = (HashMap)metaData.get(group);
           if (groupData == null) return null;
           MetaDataValue val = (MetaDataValue)groupData.get(attr);
           if (val == null) return null;
           return val.get();
           }
           catch (IOException ioex)
           {
           throw new RuntimeException("failed on MarshalledValue", ioex);
           }
           catch (ClassNotFoundException ex)
           {
           throw new RuntimeException("failed on MarshalledValue", ex);
           }
           }
          
           public synchronized void removeMetaData(Object group, Object attr)
           {
           HashMap groupData = (HashMap)metaData.get(group);
           if (groupData != null)
           {
           groupData.remove(attr);
           }
           }
          
           public synchronized void removeGroupData(Object group)
           {
           metaData.remove(group);
           }
          
           public synchronized void clear()
           {
           metaData.clear();
           }
          
           /**
           * merges incoming data. Incoming data overrides existing data
           */
           public synchronized void mergeIn(SimpleMetaData data)
           {
           Iterator it = data.metaData.keySet().iterator();
           while (it.hasNext())
           {
           Object group = it.next();
           HashMap attrs = (HashMap)data.metaData.get(group);
           HashMap map = (HashMap)metaData.get(group);
           if (map == null)
           {
           map = new HashMap();
           this.metaData.put(group, map);
           }
           map.putAll(attrs);
           }
           }
          
           public synchronized Object resolve(Invocation invocation, Object group, Object attr)
           {
           return getMetaData(group, attr);
           }
          
           public SimpleMetaData getAllMetaData(Invocation invocation)
           {
           return this;
           }
          
           public void writeExternal(java.io.ObjectOutput out)
           throws IOException
           {
           ...
           }
          
           public void readExternal(java.io.ObjectInput in)
           throws IOException, ClassNotFoundException
           {
           ...
           }
          
          }
          


          public abstract class Dispatcher
          {
           HashMap targetMap = new HashMap();
          
          
           public boolean isRegistered(Object oid)
           {
           synchronized (targetMap)
           {
           return targetMap.containsKey(oid);
           }
           }
           /**
           * Register an Object ID with an actual target object
           */
           public void registerTarget(Object oid, Object target)
           {
           synchronized (targetMap)
           {
           targetMap.put(oid, target);
           }
           }
          
           public void unregisterTarget(Object oid)
           {
           synchronized (targetMap)
           {
           targetMap.remove(oid);
           }
           }
          
           public Object getRegistered(Object oid)
           {
           synchronized (targetMap)
           {
           return targetMap.get(oid);
           }
           }
          
           public InvocationResponse invoke(Invocation invocation) throws Throwable
           {
           Invocation invocation = processInvocation();
           InvocationReponse response;
           try
           {
           Object target = findTarget(invocation);
           invocation.setTarget(target);
           Object result = dispatch(invocation);
           response = invocation.getOrCreateResponse();
           response.setResult(result);
           }
           catch (Throwable t)
           {
           response = invocation.getOrCreateResponse();
           response.setThrowable(t)
           }
           return response;
           }
          
           public abstract Invocation processInvocation();
           public abstract Object findTarget(Invocation inv, Dispatcher d);
           public abstract Object dispatch(Invocation inv) throws Throwable;
          }
          


          • 2. Re: org.jboss.ms.server: Dispatcher and Invocation impl
            Juha Lindfors Master

            The copy() method of the invocation context looks broken,

            per invocation, a new copy of the context is added. This however is not correctly copying the mutable objects, in this case the dispatcher.

            Others that should be passed by copy to the incoming thread are the descriptor, and possibly the interceptors.

            RCS file: /cvsroot/jboss/jmx/src/main/org/jboss/mx/server/InvocationContext.java,v
            
            public final void copy(final InvocationContext src)
            {
             if (sc == null)
             return;
            
             this.descriptor = src.descriptor.copy();
             this.interceptors = src.interceptors.copy();
            }
            


            The above is pseudo code. If the dispatcher will never evolve with additional state then it makes sense to move the args to the invocation. Otherwise the dispatcher should be one of the objects to copy in the above snippet. I have not verified that it actually fixes the problem you're seeing.

            The more you copy the more your throughput suffers of course. It's also a decision whether you copy (and be safe on concurrency) or try to correctly synchronize mutable invocation context (more error prone but at some point more efficient).


            • 3. Re: org.jboss.ms.server: Dispatcher and Invocation impl
              Adrian Brock Master

              I think Alex's fix make sense, especially since we will be reimplementing the
              jmx invocation to look like the aop invocation when we do the interceptor unification.

              What does it mean to have arguments associated with an invocation context
              (i.e. a call path) instead of with an invocation?

              I don't like the idea of cloning the dispatcher on every request.

              • 4. Re: org.jboss.ms.server: Dispatcher and Invocation impl
                Juha Lindfors Master

                 

                "adrian@jboss.org" wrote:

                I don't like the idea of cloning the dispatcher on every request.


                Right, but you have to either copy or synchronize anything that might be mutated by the thread that's on the context. If there's any state in the dispatcher or you want to keep the dispatcher abstraction then either one needs to be implemented.

                Same applies to any other mutable fields that is associated with the call - the metadata descriptors or the interceptors.

                Share in the context and sync the threads or make a copy. Synchronizing probably gives better single thread performance but eventually cause thread contention and might not scale up as well.

                Whether the args are in the invocation or in the dispatcher makes no difference, they're already per call context anyway, created by the caller. The problem was that the same dispatcher reference was shared with all incoming calls.