On 6 Sep 2010, at 10:52, Mircea Markus wrote: > Hi, > > This is about what happens with locking and transactions when an exception appears during an invocation. It might be a e.g. TimeoutException or an CacheStore related exception or any type of exception. > Deadlock detection code relies on this policy, so I need to clearly define it. > > Current logic is rather unclear and spreads over multiple interceptors: > - CallInterceptor would mark the tx for rollback if ant exception happen. This won't include TE as lock acquisition happen earlier in the call stack ^^ What's TE? TimeoutException?

> - InvocationContextInterceptor releases locks on all keys associated with this call, but doesn't care if the call is in a tx or not so it doesn't rollback tx or release tx locks (this logic rather belongs in LockingInterceptor) It would release tx locks specific to the invocation. Look at the impl of the InvocationContext used to retrieve the locks in question. Granted, it doesn't cause the transaction to roll back though - and IMO it should. Do you have a list of exceptional conditions which do not cause a transaction to roll back at the moment? I suppose cache store exceptions?

> - TxInterceptor and LockInterceptor just ignore exceptions You mean, pass them up higher in the interceptor chain - possibly for the InvocationContextInterceptor to deal with.

> As discussed in a previous email, the desired behaviour in case of a TimeoutException is to mark the tx for rollback. I think this can be extended as a rule for any type of Exception happening during a transaction - e.g. store exception, remote communication exception etc. On the short, if the user receives any kind of exception while writing in a tx (e.g. cache.put() throws an exception) he should know that tx was marked as rollback only. Wdyt? Yep. And the InvocationContextInterceptor is the best place to trap this and deal with it since it will have visibility of exceptions further down the chain (provided they aren't swallowed somewhere). Essentially the stuff that the CallInterceptor does when it catches an exception in command.perform() should be moved to the InvocationContextInterceptor. http://fisheye.jboss.org/browse/Infinispan/trunk/core/src/main/java/org/i...

Naturally we should (a) list all exception types we can come up with, in each interceptor (b) test behaviour on an ongoing transaction for each of these exceptional types. A single functional test should do the trick.

Cheers Manik -- Manik Surtani manik(a)jboss.org Lead, Infinispan Lead, JBoss Cache http://www.infinispan.org http://www.jbosscache.org _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On 6 Sep 2010, at 12:37, Mircea Markus wrote: > > On 6 Sep 2010, at 11:10, Manik Surtani wrote: > >> >> On 6 Sep 2010, at 10:52, Mircea Markus wrote: >> >>> Hi, >>> >>> This is about what happens with locking and transactions when an exception appears during an invocation. It might be a e.g. TimeoutException or an CacheStore related exception or any type of exception. >>> Deadlock detection code relies on this policy, so I need to clearly define it. >>> >>> Current logic is rather unclear and spreads over multiple interceptors: >>> - CallInterceptor would mark the tx for rollback if ant exception happen. This won't include TE as lock acquisition happen earlier in the call stack >> >> ^^ What's TE? TimeoutException? > yes >> >>> - InvocationContextInterceptor releases locks on all keys associated with this call, but doesn't care if the call is in a tx or not so it doesn't rollback tx or release tx locks (this logic rather belongs in LockingInterceptor) >> >> It would release tx locks specific to the invocation. Look at the impl of the InvocationContext used to retrieve the locks in question. >> >> Granted, it doesn't cause the transaction to roll back though - and IMO it should. Do you have a list of exceptional conditions which do not cause a transaction to roll back at the moment? I suppose cache store exceptions? > The ones I can think of are: > - timeout exceptions (local or remote) > - cache store exceptions > - deadlock exceptions > - replication/invalidation exceptions > - exceptions from custom interceptors. > - InvalidStateException if if call is made in the scope of a transaction that was marked for rollback >> >>> - TxInterceptor and LockInterceptor just ignore exceptions >> >> You mean, pass them up higher in the interceptor chain - possibly for the InvocationContextInterceptor to deal with. > yes >> >>> As discussed in a previous email, the desired behaviour in case of a TimeoutException is to mark the tx for rollback. I think this can be extended as a rule for any type of Exception happening during a transaction - e.g. store exception, remote communication exception etc. On the short, if the user receives any kind of exception while writing in a tx (e.g. cache.put() throws an exception) he should know that tx was marked as rollback only. Wdyt? >> >> Yep. And the InvocationContextInterceptor is the best place to trap this and deal with it since it will have visibility of exceptions further down the chain (provided they aren't swallowed somewhere). Essentially the stuff that the CallInterceptor does when it catches an exception in command.perform() should be moved to the InvocationContextInterceptor. >> >> http://fisheye.jboss.org/browse/Infinispan/trunk/core/src/main/java/org/i... > I agree that is the best solution. The drawback is the fact that we would have the locking logic spread over two interceptors instead of one - LockinInterceptor(harder to follow etc). Same with tx. These would need to be well documented. The actual logic could be wrapped up in the LockManager. You could then just invoke LockManager#releaseLocksAcquiredInCurrentInvocation(ctx) from the InvocationContextInterceptor. I notice you've ripped this logic out of the InvocationContextInterceptor in branches/4.2.x http://fisheye.jboss.org/browse/Infinispan/branches/4.2.x/core/src/main/j... Note that this is incorrect since, as I pointed out, you will miss exceptions thrown between the InvocationContextInterceptor and the LockingInterceptor.

>> Naturally we should (a) list all exception types we can come up with, in each interceptor (b) test behaviour on an ongoing transaction for each of these exceptional types. A single functional test should do the trick. > Sounds good. Any other exceptions you might have in mind? I think that should be it. Cheers Manik -- Manik Surtani manik(a)jboss.org Lead, Infinispan Lead, JBoss Cache http://www.infinispan.org http://www.jbosscache.org _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

On 6 Sep 2010, at 14:28, Mircea Markus wrote: >> I notice you've ripped this logic out of the InvocationContextInterceptor in branches/4.2.x >> >> http://fisheye.jboss.org/browse/Infinispan/branches/4.2.x/core/src/main/j... >> >> Note that this is incorrect since, as I pointed out, you will miss exceptions thrown between the InvocationContextInterceptor and the LockingInterceptor. > This code was committed before this discussion to take place. I needed it in because couldn't work on DLD without it). > It wil be addressed in ISPN-629 Ok, just be careful when rolling it back/reimplementing - it's tricky stuff. A few extra tests around it would be a good idea as well. :)

- InvocationContextInterceptor releases locks on all keys associated with this call, but doesn't care if the call is in a tx or not so it doesn't rollback tx or release tx locks (this logic rather belongs in LockingInterceptor)