11 Replies Latest reply on Jul 1, 2009 8:08 PM by kennardconsulting

    JBoss 5.1 regression: ArjunaJTA loses stack trace

    kennardconsulting

      There's some code in com.arjuna.ats.internal.jta.transaction.arjunacore.TransactionImple that says...

      // keep a record of why we are rolling back i.e. who called us first, it's a useful debug aid.
      if(_rollbackOnlyCallerStacktrace == null)
      {
       _rollbackOnlyCallerStacktrace = new Throwable("setRollbackOnly called from:");
      }


      ...this seems to partner with some other code that says...

      if(_rollbackOnlyCallerStacktrace != null) {
       // we rolled back beacuse the user explicitly told us not to commit. Attach the trace of who did
      that for debug:
       rollbackException.initCause(_rollbackOnlyCallerStacktrace);
      }
      else if(_theTransaction.getDeferredThrowable() != null) {
       // we tried to commit but it went wrong - attach the reason for debug:
       rollbackException.initCause(_theTransaction.getDeferredThrowable());
      }
      throw rollbackException;


      ...so we are attaching an empty Throwable with the text 'setRollbackOnly called from:' to the bottom of the stack trace. Contrary to the comments, this does not appear a useful debug aid. Rather, the 'deferredThrowable' might be more useful.

      In JBoss 4.2.3.GA the bottom of the stacktrace contained a useful cause (such as the SQL error that caused the rollback), rather than just the text 'setRollbackOnly called from:'

      Is this a bug?

      Regards,

      Richard.

        • 1. Re: JBoss 5.1 regression: ArjunaJTA loses stack trace
          jhalliday

          There are two rollback cases at work here: rollback because an exception was thrown and rollback becuase the user explicitly called setRollbackOnly.

          In older version there was no logging of the setRollbackOnly case, making it hard to find which code, often a 3rd party library, was making the call. The new version tries to help with that.

          You seem to be saying that the deferredThrowable, if any, should take precedence over the callerStackTrace? That will work in many cases, but is wrong where other code catches e.g. SQLException, calls setRollbackOnly, then rethrows the SQLException.

          I don't think there is a way to please everybody, but I'm willing to consider alternatives if you can give me a more clear example of why the current model does not do what you want.

          • 2. Re: JBoss 5.1 regression: ArjunaJTA loses stack trace
            kennardconsulting

            jhalliday,

            Thank you for being so responsive, and so open to suggestions.

            I agree that, from a debugging perspective, your code increases log output and therefore makes things easier to debug. The original SQLException is still in the log (a little higher up), and the new rollback exception is probably very useful too.

            The problem, in my case, is I am not using this exception for debugging - I am using it in my application code. I drill into the exception to find the root cause to try and display something meaningful to the user. For example, if I find a SQLException at the bottom I can examine it and display something like 'item already exists' or 'item is relied upon by other items'. This is not foolproof, I know, but it works well enough for my purposes.

            In many ways I don't care about the extra setRollbackOnly exception, but I care that it terminates the stack trace. If it instead did (pseudo-code)...

            // keep a record of why we are rolling back i.e. who called us first, it's a useful debug aid.
            if(_rollbackOnlyCallerStacktrace == null)
            {
             _rollbackOnlyCallerStacktrace = new Throwable("setRollbackOnly called from:", _theTransaction.getDeferredThrowable());
            }


            ...(ie. still cites the original exception as its cause) that would be fine.

            Regards,

            Richard.

            • 3. Re: JBoss 5.1 regression: ArjunaJTA loses stack trace
              jhalliday

              Mmm, interesting use case. We normally think of the public API as being packages, classes, method signatures and such. We pay less attention to the stability between versions of the exception's content. So what you are doing is inherently brittle, but in this particular case I see no problem changing the code to accommodate you. I'll get to it when I'm back from Jazoon.

              • 4. Re: JBoss 5.1 regression: ArjunaJTA loses stack trace
                ben.cotton2

                Excellent. Accommodating an "inherently brittle" bounded sequence (be they CRUD statements, exception content's versions, services, etc.) should be the over-riding primary mission of any transaction manager provider. thanks, indeed.

                • 5. Re: JBoss 5.1 regression: ArjunaJTA loses stack trace
                  kennardconsulting

                  Ben:

                  Your comment seems unhelpful. jhalliday explicitly said they don't normally accomodate 'inherently brittle' sequences, but 'in this particular case' he may.

                  jhalliday:

                  Thanks for being open to making the change.

                  While I appreciate what I am doing is somewhat brittle, I don't know I agree that exceptions aren't part of the public API. Checked exceptions are clearly part of the public API. And unchecked exceptions should be too, albeit not compiler-enforced.

                  It is a small step from there to say that exception 'causes' are part of the public API too. For example, Hibernate always throws a HibernateException for everything, and HibernateException includes a 'cause' (it extends NestableRuntimeException). If the next release of Hibernate suddenly nulled out all causes of all HibernateExceptions, I'll wager a lot of code would break.

                  Note you and I have been here before - https://jira.jboss.org/jira/browse/JBTM-66 :)

                  • 6. Re: JBoss 5.1 regression: ArjunaJTA loses stack trace
                    kennardconsulting

                    You've been here before with this guy too: https://jira.jboss.org/jira/browse/JBAS-4238

                    • 7. Re: JBoss 5.1 regression: ArjunaJTA loses stack trace
                      ben.cotton2

                       

                      jhalliday explicitly said they don't normally accomodate 'inherently brittle' sequences, but 'in this particular case' he may.


                      But what is implicitly true, is that the most important thing any transaction manager does is accommodate 'inherently brittle' sequences -namely transactions! It is just in the nature of a TM service developer to accommodate the "inherently brittle" ... it is logical to do so ... it is important to do so ... it is necessary to do so.

                      The exact symmetry of logic exists for accommodating your use case ... it is necessary to do so.

                      • 8. Re: JBoss 5.1 regression: ArjunaJTA loses stack trace
                        jhalliday

                        > I don't know I agree that exceptions aren't part of the public API. Checked exceptions are clearly part of the public API.

                        Yes of course they are. But that's just their type, not their content. The exception that is thrown is still RollbackException, that is not changed. You are bitching about the exception that is chained from it, which is for information only and which is not constrained by the API spec, either ours or the JTA.

                        As far as I can see, the only situation that's going to cause problems with the current code, is a tx that attempts to commit, gets an exception from a beforeCompletion (e.g. hibernate fails to flush state to the db) AND gets setRollbackOnly called on it at a point in time after it has begun to terminate. Arguably that's misuse - either throw an exception from the Synchronization and rely on the tx manager to catch it and rollback, or catch it yourself and call setRollbackOnly on the tx. However, given the variation in behaviour of tx mgr impls, I accept its reasonable for the 3rd party code to be catching the exception, calling setRollbackOnly, then rethrowing the exception just in case. In such case, it seems to me the fix is

                        if(_rollbackOnlyCallerStacktrace == null) {
                         if(_deferredThrowable != null) {
                         _rollbackOnlyCallerStacktrace = new Throwable("setRollbackOnly called from here, possibly due to earlier error as below: ");
                         _rollbackOnlyCallerStacktrace.initCause(_deferredThrowable);
                         } else {
                         _rollbackOnlyCallerStacktrace = new Throwable("setRollbackOnly called from here: ");
                         }
                        }
                        


                        which should preserve all the relevant information. The only problem I can see is that it may be misleading in the unlikely case that setRollbackOnly is called for a reason unrelated to the exception thrown from beforeCompletion. The alternative is to change the precedence so that the deferredThrowable takes priority over the _rollbackOnly:

                        if( _theTransaction.getDeferredThrowable != null) {
                         rollbackException.initCause(_theTransaction.getDeferredThrowable());
                        } else if(_rollbackOnlyCallerStackTrace != null) {
                         rollbackException.initCause(_rollbackOnlyCalledStackTrace)
                        }
                        


                        which has the problem that any useful information in the callerStackTrace is lost, although only in cases where there is a potentially more interesting exception.

                        • 9. Re: JBoss 5.1 regression: ArjunaJTA loses stack trace
                          kennardconsulting

                          jhalliday,

                          My apologies - I am absolutely not bitching. I'm sorry if it sounds that way. I'm grateful for everything you guys do and I'm trying to ask politely.

                          I'm simply saying that, for my use case, it would be very useful to have a rollback exception always contain information about its cause.

                          Regards,

                          Richard.

                          • 10. Re: JBoss 5.1 regression: ArjunaJTA loses stack trace
                            kennardconsulting

                            Would you like me to open a JIRA to track this?

                            • 11. Re: JBoss 5.1 regression: ArjunaJTA loses stack trace
                              kennardconsulting