OpenJPA
  1. OpenJPA
  2. OPENJPA-343

Do not call setRollbackOnly on inactive Transactions

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.9.7, 1.0.0
    • Fix Version/s: 1.0.1, 1.1.0
    • Component/s: kernel
    • Labels:
      None

      Description

      While in the middle of processing an afterCompletion invocation in BrokerImpl, an unexpected RuntimeException (IndexOutOfBoundsException) occurred within some underlying WebSphere code. While we (OpenJPA) were attempting to clean up after that exception, we attempted to call setRollbackOnly on the current transaction. But, since we were in the process of completing the current transaction, it is invalid to be calling setRollbackOnly and we ended up getting an IllegalStateException from the WebSphere Transaction Manager. Due this second exception, we ended up losing track of the original exception and this became a difficult problem to diagnose.

      This issue will be used to correct a couple of issues (at least):

      1) We should ensure that the transaction is active before calling
      setRollbackOnly(). When an exception happens during afterCompletion
      processing, the Transaction can no longer accept setRollbackOnly
      invocations.

      2) When an unexpected exception happens like this, we should log the
      exception before attempting to process the exception. In this particular
      case, we lost the original exception when we ran into the IllegalStateException
      from the Transaction service. This forced us to re-run the scenario just to
      get a trace of the exception.

      3) Or, if we don't want to log the exception immediately, we need to determine why we lost the first exception in the first place and ensure that doesn't happen again.

      Kevin

        Activity

        Hide
        Kevin Sutter added a comment -

        Resolved in trunk (1.1.0) via svn revision #571523.

        Show
        Kevin Sutter added a comment - Resolved in trunk (1.1.0) via svn revision #571523.
        Hide
        Kevin Sutter added a comment -

        > Here's where we might disagree. The user-level commit should fail so the user doesn't think everything is ok as far as the cache is concerned. I understand that the database transaction is complete and whatever changes have been made there are permanent. But the EntityManager is possibly corrupted.

        I agree. We are performing this "afterCompletion" processing as far as we can until we hit this unexpected exception. This exception is now being logged (if trace is turned on) and the exception is returned to the caller. Unless you are suggesting that we should possibly log-and-eat this exception and attempt to continue additional processing as if nothing has happened, I think we have done everything we can do. If we went this log-and-eat route, it would require more more granular try-catch blocks in this path. Not sure this is necessary processing for the unexpected (rare) case.

        FYI, we are still flowing through the EntityManager with the setRollbackOnly invocation. So, any processing in the EntityManager and/or Broker that would be triggered because of the setRollbackOnly call will still happen. It's just the explicit setRollbackOnly call on the Transaction object itself that was conditionally skipped if the Transaction was not "active". Just wanted to clarify that aspect of the change.

        If there are still concerns about the general exception processing in the OpenJPA code base, then maybe we should open a separate JIRA issue or at least start a [DISCUSSION] topic on our dev mailing list. The original intent of this Issue was to not call the setRollbackOnly method when the Transaction is not in a state to accept the invocation. I think that issue has been resolved. But, maybe there are still concerns about the general exception processing within OpenJPA...

        Thanks,
        Kevin

        Show
        Kevin Sutter added a comment - > Here's where we might disagree. The user-level commit should fail so the user doesn't think everything is ok as far as the cache is concerned. I understand that the database transaction is complete and whatever changes have been made there are permanent. But the EntityManager is possibly corrupted. I agree. We are performing this "afterCompletion" processing as far as we can until we hit this unexpected exception. This exception is now being logged (if trace is turned on) and the exception is returned to the caller. Unless you are suggesting that we should possibly log-and-eat this exception and attempt to continue additional processing as if nothing has happened, I think we have done everything we can do. If we went this log-and-eat route, it would require more more granular try-catch blocks in this path. Not sure this is necessary processing for the unexpected (rare) case. FYI, we are still flowing through the EntityManager with the setRollbackOnly invocation. So, any processing in the EntityManager and/or Broker that would be triggered because of the setRollbackOnly call will still happen. It's just the explicit setRollbackOnly call on the Transaction object itself that was conditionally skipped if the Transaction was not "active". Just wanted to clarify that aspect of the change. If there are still concerns about the general exception processing in the OpenJPA code base, then maybe we should open a separate JIRA issue or at least start a [DISCUSSION] topic on our dev mailing list. The original intent of this Issue was to not call the setRollbackOnly method when the Transaction is not in a state to accept the invocation. I think that issue has been resolved. But, maybe there are still concerns about the general exception processing within OpenJPA... Thanks, Kevin
        Hide
        Craig L Russell added a comment -

        > So, when an exception happens in this code path, it could mean that some of our cleanup didn't complete. We do log any exceptions that happen during this path via trace statements and the exception does get re-thrown.

        > There is nothing we can do at this point to revert the state of that transaction. It was already either committed or rolled back by the time the afterCompletion method is invoked.

        I agree.

        > The parameter on afterCompletion lets you know whether the transaction committed or rolled back. So, there is nothing to "fail".

        Here's where we might disagree. The user-level commit should fail so the user doesn't think everything is ok as far as the cache is concerned. I understand that the database transaction is complete and whatever changes have been made there are permanent. But the EntityManager is possibly corrupted.

        Craig

        Show
        Craig L Russell added a comment - > So, when an exception happens in this code path, it could mean that some of our cleanup didn't complete. We do log any exceptions that happen during this path via trace statements and the exception does get re-thrown. > There is nothing we can do at this point to revert the state of that transaction. It was already either committed or rolled back by the time the afterCompletion method is invoked. I agree. > The parameter on afterCompletion lets you know whether the transaction committed or rolled back. So, there is nothing to "fail". Here's where we might disagree. The user-level commit should fail so the user doesn't think everything is ok as far as the cache is concerned. I understand that the database transaction is complete and whatever changes have been made there are permanent. But the EntityManager is possibly corrupted. Craig
        Hide
        Kevin Sutter added a comment -

        > If there's nothing important for OpenJPA to do in afterCompletion, then it should simply return and there cannot be an exception. If there is something important to do, then it's important that it complete.

        > Is this not true?

        Yes, that is true. I was typing before thinking. Our afterCompletion processing does do some "real work" with ending the transaction, detaching entities, etc. So, when an exception happens in this code path, it could mean that some of our cleanup didn't complete. We do log any exceptions that happen during this path via trace statements and the exception does get re-thrown. It may get wrapped in a suitable PersistenceException, but it will get sent back.

        But, to get back to what I was attempting to explain... The transaction itself is already completed. There is nothing we can do at this point to revert the state of that transaction. It was already either committed or rolled back by the time the afterCompletion method is invoked. The beforeCompletion and afterCompletion methods on the Synchronization interface are just convenient listener methods. Any exceptions that may happen due to these calls will not affect the outcome of the Transaction (since it is already complete). And, from my experience, the Synchronization implementation will not stop processing just because an exception happens. They will log the condition (as we have seen) and then continue processing.

        Hope this helps.
        Kevin

        Show
        Kevin Sutter added a comment - > If there's nothing important for OpenJPA to do in afterCompletion, then it should simply return and there cannot be an exception. If there is something important to do, then it's important that it complete. > Is this not true? Yes, that is true. I was typing before thinking. Our afterCompletion processing does do some "real work" with ending the transaction, detaching entities, etc. So, when an exception happens in this code path, it could mean that some of our cleanup didn't complete. We do log any exceptions that happen during this path via trace statements and the exception does get re-thrown. It may get wrapped in a suitable PersistenceException, but it will get sent back. But, to get back to what I was attempting to explain... The transaction itself is already completed. There is nothing we can do at this point to revert the state of that transaction. It was already either committed or rolled back by the time the afterCompletion method is invoked. The beforeCompletion and afterCompletion methods on the Synchronization interface are just convenient listener methods. Any exceptions that may happen due to these calls will not affect the outcome of the Transaction (since it is already complete). And, from my experience, the Synchronization implementation will not stop processing just because an exception happens. They will log the condition (as we have seen) and then continue processing. Hope this helps. Kevin
        Hide
        Patrick Linskey added a comment -

        I'm generally against the log-and-throw pattern, whenever it's possible to avoid it. When it is necessary, I think that it's important that the log only happen at the trace level, so that we aren't unnecessarily repeating exception printouts.

        This conversation seems to have diverged a bit from suggesting a log-and-throw, but I figured it was worth mentioning anyways.

        Show
        Patrick Linskey added a comment - I'm generally against the log-and-throw pattern, whenever it's possible to avoid it. When it is necessary, I think that it's important that the log only happen at the trace level, so that we aren't unnecessarily repeating exception printouts. This conversation seems to have diverged a bit from suggesting a log-and-throw, but I figured it was worth mentioning anyways.
        Hide
        Craig L Russell added a comment -

        > When we get to this spot, the Transaction is complete. All of the prepares and commits and/or rollbacks have completed. The afterCompletion call is just a convenience callback to let those resources that have registered for Synchronization that the transaction has completed. The parameter on afterCompletion lets you know whether the transaction committed or rolled back. So, there is nothing to "fail".

        If there's nothing important for OpenJPA to do in afterCompletion, then it should simply return and there cannot be an exception. If there is something important to do, then it's important that it complete.

        Is this not true?

        Show
        Craig L Russell added a comment - > When we get to this spot, the Transaction is complete. All of the prepares and commits and/or rollbacks have completed. The afterCompletion call is just a convenience callback to let those resources that have registered for Synchronization that the transaction has completed. The parameter on afterCompletion lets you know whether the transaction committed or rolled back. So, there is nothing to "fail". If there's nothing important for OpenJPA to do in afterCompletion, then it should simply return and there cannot be an exception. If there is something important to do, then it's important that it complete. Is this not true?
        Hide
        Kevin Sutter added a comment -

        > I guess the issue is whether this is a trace scenario or a more serious problem that should be reported back. We are still in commit as far as the application is concerned and it's not obvious to me that this is a successful transaction. I'd think we should cause the outer level transaction to fail with a SystemException because the application handling is not consistent (the cache, for example, might be in an inconsistent state). If the application thinks everything is aok, then I think we have a problem.

        When we get to this spot, the Transaction is complete. All of the prepares and commits and/or rollbacks have completed. The afterCompletion call is just a convenience callback to let those resources that have registered for Synchronization that the transaction has completed. The parameter on afterCompletion lets you know whether the transaction committed or rolled back. So, there is nothing to "fail".

        And, actually, the IllegalStateException that I mentioned about we get when we attempt to call setRollbackOnly is only logged by the TM. It is not thrown back to the caller since there is nothing to fail. So, the application actually continues running without a problem. The exception just gets logged so that we know that something isn't quite right, but everything still completed "okay".

        Show
        Kevin Sutter added a comment - > I guess the issue is whether this is a trace scenario or a more serious problem that should be reported back. We are still in commit as far as the application is concerned and it's not obvious to me that this is a successful transaction. I'd think we should cause the outer level transaction to fail with a SystemException because the application handling is not consistent (the cache, for example, might be in an inconsistent state). If the application thinks everything is aok, then I think we have a problem. When we get to this spot, the Transaction is complete. All of the prepares and commits and/or rollbacks have completed. The afterCompletion call is just a convenience callback to let those resources that have registered for Synchronization that the transaction has completed. The parameter on afterCompletion lets you know whether the transaction committed or rolled back. So, there is nothing to "fail". And, actually, the IllegalStateException that I mentioned about we get when we attempt to call setRollbackOnly is only logged by the TM. It is not thrown back to the caller since there is nothing to fail. So, the application actually continues running without a problem. The exception just gets logged so that we know that something isn't quite right, but everything still completed "okay".
        Hide
        Craig L Russell added a comment -

        I agree that 1) not calling setRollbackOnly during afterCompletion is correct.

        But when 2) an unexpected exception occurs during afterCompletion, like a NullPointerException, it seems that we should log the exception. Even though it's during afterCompletion, we shouldn't just swallow it. Of course, fixing 1) means that 2) can't be reliably reproduced.

        I'm not clear on what 3) means.

        > I plan to resolve issue (1) by checking for a valid transaction status before calling setRollbackOnly. If the transaction is not in a suitable state, I will log a trace message indicating that the setRollbackOnly can not be called, but processing will continue.

        I guess the issue is whether this is a trace scenario or a more serious problem that should be reported back. We are still in commit as far as the application is concerned and it's not obvious to me that this is a successful transaction. I'd think we should cause the outer level transaction to fail with a SystemException because the application handling is not consistent (the cache, for example, might be in an inconsistent state). If the application thinks everything is aok, then I think we have a problem.

        Craig

        Show
        Craig L Russell added a comment - I agree that 1) not calling setRollbackOnly during afterCompletion is correct. But when 2) an unexpected exception occurs during afterCompletion, like a NullPointerException, it seems that we should log the exception. Even though it's during afterCompletion, we shouldn't just swallow it. Of course, fixing 1) means that 2) can't be reliably reproduced. I'm not clear on what 3) means. > I plan to resolve issue (1) by checking for a valid transaction status before calling setRollbackOnly. If the transaction is not in a suitable state, I will log a trace message indicating that the setRollbackOnly can not be called, but processing will continue. I guess the issue is whether this is a trace scenario or a more serious problem that should be reported back. We are still in commit as far as the application is concerned and it's not obvious to me that this is a successful transaction. I'd think we should cause the outer level transaction to fail with a SystemException because the application handling is not consistent (the cache, for example, might be in an inconsistent state). If the application thinks everything is aok, then I think we have a problem. Craig
        Hide
        Kevin Sutter added a comment -

        I should clarify a couple of statements. I mixed Transaction and Synchronization concepts in the above description. Since we are in the afterCompletion (Synchronization) processing, the Transaction is in a "completed" state and can no longer accept the setRollbackOnly invocation. Although related, Transaction and Synchronization are two separate concepts and should treated as such.

        I plan to resolve issue (1) by checking for a valid transaction status before calling setRollbackOnly. If the transaction is not in a suitable state, I will log a trace message indicating that the setRollbackOnly can not be called, but processing will continue.

        Re-looking at the code, I think we are already sufficiently addressing items (2) and (3), once issue (1) is resolved. We already have the necessary trace logging for this unexpected exception. Granted, we didn't expect this exception and it's a "pain" to turn on trace for long-running scenarios, the real exception that caused the problem in the first place will now get logged due to the fix for (1). And, since OpenJPA doesn't have the concept of a first-failure-data-capture logging facility, it would get cumbersome to log every unexpected exception.

        So, my plans at this point are to just resolve the first issue (1) with ensuring that we do not attempt to call setRollbackOnly when the transaction can not accept it. Comments welcome.

        Kevin

        Show
        Kevin Sutter added a comment - I should clarify a couple of statements. I mixed Transaction and Synchronization concepts in the above description. Since we are in the afterCompletion (Synchronization) processing, the Transaction is in a "completed" state and can no longer accept the setRollbackOnly invocation. Although related, Transaction and Synchronization are two separate concepts and should treated as such. I plan to resolve issue (1) by checking for a valid transaction status before calling setRollbackOnly. If the transaction is not in a suitable state, I will log a trace message indicating that the setRollbackOnly can not be called, but processing will continue. Re-looking at the code, I think we are already sufficiently addressing items (2) and (3), once issue (1) is resolved. We already have the necessary trace logging for this unexpected exception. Granted, we didn't expect this exception and it's a "pain" to turn on trace for long-running scenarios, the real exception that caused the problem in the first place will now get logged due to the fix for (1). And, since OpenJPA doesn't have the concept of a first-failure-data-capture logging facility, it would get cumbersome to log every unexpected exception. So, my plans at this point are to just resolve the first issue (1) with ensuring that we do not attempt to call setRollbackOnly when the transaction can not accept it. Comments welcome. Kevin

          People

          • Assignee:
            Kevin Sutter
            Reporter:
            Kevin Sutter
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development