4 Replies Latest reply on Dec 21, 2005 12:36 AM by ovidiu.feodorov

    Stateful vs. stateless interceptors

    ovidiu.feodorov

      The current Messaging implementation uses client-side dynamic proxies to implement delegates (ConnectionDelegate, SessionDelegate, aso). Their invocation handlers contain arrays of AOP interceptors. The dynamic proxies are constructed on the server and shipped on the client each time a new top-level JMS API element is created. They may be either serialized if the client that requests their creation is remote, or just passed by reference, if the client and the server are co-located.

      The latest decision relative to how to handle the state for client-side delegates was to use stateful interceptors. Relevant state is stored by corresponding interceptors. Case in point: the TransactedInterceptor stores whether a session is transacted or not, by setting its "transacted" protected member. Obviously, this implies with necessity that each session delegate must have its unique TransactedInterceptor instance. If not, creating more than one session per VM would lead to corrupted state. This is what unfortunately happens now.

      Run SessionTest.testCreateTwoSessions() in a co-located configuration to get proof. Another test that exhibits the same behavior is SessionTest.testCloseAndCreateSession(). Both tests currently fail (in a co-located configuration). For a non co-located configuration, the tests will pass, because (I assume) the PER_CLASS semantics is not maintained over serialization, so each delegate ends with its own interceptor instance (to be confirmed).

      I see at least two ways to fix the problem.

      1. Keep the interceptors stateful, but declare them PER_INSTANCE instead of PER_CLASS in jms-aop.xml. I think this solution may be made to work, but there are a few complications that must be resolved first: declaring an interceptor PER_INSTANCE will result in the creation of a PerInstanceInterceptor which delegates to our interceptor only if the invocation has a non-null targetObject (otherwise the invocation is considered static). So when JMSInvocationHandler creates the invocation, we need some sort of targetObject, which implements Advised and it's able to return an InstanceAdvisor, aso. I haven't explored this all the way to the end, but it already looks complicated to me. It also implies a lot of new object instances creations per request.

      2. Make the interceptors stateless. Delegate state management to JMSInvocationHandler instances. Arguments in favor of this decision:


      * There is a natural one-to-one relationship between delegate instances and their invocation handlers. We don't need to write/configure anything to enforce this.
      * We can easily type invocation handlers (we are already using a JMSConsumerInvocationHandler). This will make the whole architecture easier to understand.
      * We can easily add typed getters/setters to take the pain out of the interaction with the detyped metadata


      So, instead keeping the "transacted" flag inside the TransactedInterceptor, we declare a SessionInvocationHandler with a getTransacted()/setTransacted(boolean).

      The interceptor code will look similar to:

      public class TransactionInterceptor implements Interceptor, Serializable
      {
       // Attributes ----------------------------------------------------
      
       // nothing ....
      
       // Interceptor implementation ------------------------------------
      
       ...
      
      
       public Object invoke(Invocation invocation) throws Throwable
       {
       if (!(invocation instanceof JMSMethodInvocation))
       {
       return invocation.invokeNext();
       }
      
       JMSMethodInvocation mi = (JMSMethodInvocation)invocation;
       String methodName = mi.getMethod().getName();
      
       if ("setTransacted".equals(methodName))
       {
       boolean t = ((Boolean)mi.getArguments()[0]).booleanValue();
       ((SessionInvocationHandler)mi.getHandler()).setTransacted(t);
       return null;
      
       }
       ...
       }
       ...
      }
      




        • 1. Re: Stateful vs. stateless interceptors
          timfox

           

          "ovidiu.feodorov@jboss.com" wrote:
          The current Messaging implementation uses client-side dynamic proxies to implement delegates (ConnectionDelegate, SessionDelegate, aso). Their invocation handlers contain arrays of AOP interceptors. The dynamic proxies are constructed on the server and shipped on the client each time a new top-level JMS API element is created. They may be either serialized if the client that requests their creation is remote, or just passed by reference, if the client and the server are co-located.

          The latest decision relative to how to handle the state for client-side delegates was to use stateful interceptors. Relevant state is stored by corresponding interceptors. Case in point: the TransactedInterceptor stores whether a session is transacted or not, by setting its "transacted" protected member. Obviously, this implies with necessity that each session delegate must have its unique TransactedInterceptor instance. If not, creating more than one session per VM would lead to corrupted state. This is what unfortunately happens now.

          Run SessionTest.testCreateTwoSessions() in a co-located configuration to get proof. Another test that exhibits the same behavior is SessionTest.testCloseAndCreateSession(). Both tests currently fail (in a co-located configuration). For a non co-located configuration, the tests will pass, because (I assume) the PER_CLASS semantics is not maintained over serialization, so each delegate ends with its own interceptor instance (to be confirmed).

          "tim.fox@jboss.com" wrote:
          None of the AOP scoping semantics are maintained after serialization, since all we're serializing is an array of vanilla interceptors - they're not advised classes - so, in effect delegate (on the client side) gets it's own unique interceptor stack



          I see at least two ways to fix the problem.

          1. Keep the interceptors stateful, but declare them PER_INSTANCE instead of PER_CLASS in jms-aop.xml.

          I think this solution may be made to work, but there are a few complications that must be resolved first: declaring an interceptor PER_INSTANCE will result in the creation of a PerInstanceInterceptor which delegates to our interceptor only if the invocation has a non-null targetObject (otherwise the invocation is considered static).

          "tim.fox@jboss.com" wrote:
          The target object on the client side is a basically a simple object that implements the delegate interface and makes the remote invocation - nothing else. The target object on the server side is the concrete server delegate instance. This allows us to remove all that InstanceInterceptor if..else statement stuff too.

          I actually knocked up a prototype for this some time ago... I'll try and dig it out


          2. Make the interceptors stateless. Delegate state management to JMSInvocationHandler instances. Arguments in favor of this decision:

          * There is a natural one-to-one relationship between delegate instances and their invocation handlers. We don't need to write/configure anything to enforce this.

          "tim.fox@jboss.com" wrote:


          IMHO state should be stored in different places, depending on the nature of the state.

          If you are advising an object, then it is natural for that object to store state that truly represents attributes/other data for that object.
          Then on top of that, you have cross-cutting concerns, represented by the aspects (interceptors in our case), which may also need to store state, depending on what the aspect represents.

          Some aspects e.g. a LoggingInterceptor would make sense to be stateless, but others e.g. ClosedInterceptor wouldn't be since it holds the state of whether the object is closed or not, and can be applied to many different classes. (ClosedInterceptor is applied to Session, Connection, Consumer, Producer.

          So what I am saying is that there is some state that is natural to store in the advised objects themselves, but other state that doesn't have a natural home in those objects, since it is the state of a cross cutting concern. This is one of the problems that AOP approaches solve IMHO.

          So limiting us to just storing state in objects representing the advised classes (i.e. domain objects) is denying some of the benefits of AOP.





          * We can easily type invocation handlers (we are already using a JMSConsumerInvocationHandler). This will make the whole architecture easier to understand.
          * We can easily add typed getters/setters to take the pain out of the interaction with the detyped metadata

          "tim.fox@jboss.com" wrote:


          Strawman? Data can be strongly types whether or not interceptors are stateful or stateless.




          So, instead keeping the "transacted" flag inside the TransactedInterceptor, we declare a SessionInvocationHandler with a getTransacted()/setTransacted(boolean).

          The interceptor code will look similar to:

          public class TransactionInterceptor implements Interceptor, Serializable
          {
           // Attributes ----------------------------------------------------
          
           // nothing ....
          
           // Interceptor implementation ------------------------------------
          
           ...
          
          
           public Object invoke(Invocation invocation) throws Throwable
           {
           if (!(invocation instanceof JMSMethodInvocation))
           {
           return invocation.invokeNext();
           }
          
           JMSMethodInvocation mi = (JMSMethodInvocation)invocation;
           String methodName = mi.getMethod().getName();
          
           if ("setTransacted".equals(methodName))
           {
           boolean t = ((Boolean)mi.getArguments()[0]).booleanValue();
           ((SessionInvocationHandler)mi.getHandler()).setTransacted(t);
           return null;
          
           }
           ...
           }
           ...
          }
          


          "tim.fox@jboss.com" wrote:


          This makes sense for a lot of data but not all. Some data really is represented better as state of an aspect IMHO.

          Also if we make all aspects stateless then we're going to have to do extra lookups to retrieve data that they need.

          In general, I think we should not be too sweeping and say everything must be either stateful or stateless - it depends on a case by case basis.

          Having said all that, I think the biggest issue we have is that JBossAOP doesn't currently support serializing/deserializing advised objects and preserving the scoping semantics.

          We can solve this by advising the client objects on the client rather than on the server, which was suggested some time ago. This means transporting the AOP config from server to client when the connection is created. This may have problems in itself.

          Alternatively we stick with vanilla interceptors, and make everything stateless as Ovidiu has suggested, which we know will work, but IMHO is a shame since we're not leveraging the power of AOP, plus we'll still end up with the following kind of code, yuck:

          
           }
           else if ("setResourceManager".equals(methodName))
           {
           //
           }
           else if ("getTransacted".equals(methodName))
           {
           //
           }
           else if ("getXA".equals(methodName))
           {
           //
          
          







          • 2. Re: Stateful vs. stateless interceptors
            ovidiu.feodorov

            My main concern is that now, current implementation only works correctly in a remote configuration. As soon as we co-locate the server and the client and create more than one session, for example, it starts to break.

            • 3. Re: Stateful vs. stateless interceptors
              ovidiu.feodorov
              • 4. Re: Stateful vs. stateless interceptors
                ovidiu.feodorov

                Congratulations Tim,

                The facade code looks better now, indeed. Good job!

                I refactored again to change some names and apply some minor optimizations. Please look at diffs and see if they fit your vision.