Bug 886850

Summary: 4.6 on as7: different rollback semantic for Exceptions
Product: [Other] RHQ Project Reporter: Heiko W. Rupp <hrupp>
Component: Core ServerAssignee: Jay Shaughnessy <jshaughn>
Status: CLOSED CURRENTRELEASE QA Contact: Mike Foley <mfoley>
Severity: high Docs Contact:
Priority: high    
Version: 4.5CC: hrupp, jshaughn
Target Milestone: ---   
Target Release: RHQ 4.6   
Hardware: All   
OS: All   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2013-09-03 14:46:31 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Heiko W. Rupp 2012-12-13 11:17:42 UTC
I have the following code block in UserHandlerBean, that worked for a long time in as4-based rhq:


        for (Integer id : favIds) {
            try {
                    res = resourceManager.getResource(caller,id);

                    ResourceWithType rwt = fillRWT(res,uriInfo);
                    ret.add(rwt);
            }
            catch (Exception e) {
                if (e.getCause()!=null && e.getCause() instanceof ResourceNotFoundException)
                    log.debug("Favorite resource with id "+ id + " not found - not returning to the user");
                else
                    log.warn("Retrieving resource with id " + id + " failed: " + e.getLocalizedMessage());
            }
        }

This is meant to loop over all resources found in a favorite entry and to only add the vaild ones to the result(*)

What happens now on 4.6-on-as7 is that the first ResourceManager.getResource() that is passed an id of a non-existing resource will  throw the ResourceNotFoundException *wrapped* as a javax.ejb.EJBTransactionRolledbackException, which then rolls back the whole transaction, so that subsequent calls to ResourceManager.getResource() will fail with a 
javax.ejb.EJBTransactionRolledbackException: org.hibernate.exception.GenericJDBCException: Could not open connection
so basically I have to do a EntityManager.find(Resource.class,id) before the ResourceManager.getResource() call to check for valid resources to work around.

If I mark the ResourceNotFoundException with @ApplicationException, the exception 'e' is now the ResourceNotFoundException itself and the transaction is not rolled back and my above code works as on as4-based RHQ.

I am not sure if globally marking all our exceptions as @ApplicationException it the right thing, but from my EJB3-book-writing background, I think what we have done before falls more on the "how could that ever work" category.
@ApplicationException is thought to be used especially to mark the non-checked RuntimeExceptions as such that come from the application as opposed to pure RuntimeExceptions, that come from the container ("the runtime environment"), by still not forcing to declare them all over the place as checked exceptions






*) A workaround for this is not to accept invalid ones, but this does

Comment 1 Jay Shaughnessy 2013-01-02 20:26:06 UTC
I confirmed this behavior and can only conclude that 4.2.3 did not handle these exceptions correctly, whereas AS7 is behaving as per the spec.

After some discussion, the approach we'll take is to do what Heiko mentions above and annotate the exception class with @ApplicationException(rollback="false"), while leaving it a RuntimeException.

This will basically maintain the previous behavior, which we think is the intended behavior.  In general, we want these exceptions to provide information while not necessarily causing failure of an ongoing transaction.  It can be wrapped and/or rethrown as necessary by a caller, if necessary.

Comment 2 Jay Shaughnessy 2013-01-02 20:27:18 UTC
Also, this affects several other Exception classes.  I'll have to go through the various classes and uses to ensure we're getting this right...

Comment 3 Jay Shaughnessy 2013-01-04 14:19:24 UTC
master commit c29e6ace53f377cbc25349524265c3c5e59180ad
Author: Jay Shaughnessy <jshaughn>
Date:   Fri Jan 4 09:07:25 2013 -0500

It seems AS 4.2.3 did not wrap RuntimeExceptions as EJBExceptions as
per the spec. Neither did it rollback a supported transaction. Furthermore,
RHQ code was written assuming the bad semantics were correct. AS7 corrects
the handling and therefore the RHQ code is now in error in certain places.

AS 4.2.3 basically treated the RuntimeException as an ApplicationException. This
commit tries to get sime consistency around our Exception classes that should
be treated this way.  It adds the @ApplicationException annotation to several
classes, and a few others are changed from extending Exception to extending
RuntimeException as well as then being annotated.

A test has been added that ensures handling of the relevant exception classes
works as expected, as an ApplicationException that is not wrapped and does
not rollback a supported transaction.

Also:
- revert changes to UserHandlerBean to work around the problem reported in this BZ
- fix catch statement in ResourceOperationJob to catch thre ApplicationException,
  not the wrapped EJBException
- fix issue in OperationManagerBean to throw the proper ApplicationException


Test Notes
----------
This is an implicit change that is not easily explicitly tested.  As long as the REST API tests, and server itests, are passing that is probably sufficient.

Heiko, please review this commit and make sure you're satisfied. Thanks, Jay.

Comment 4 Heiko W. Rupp 2013-01-07 14:09:46 UTC
Jay, 
thanks for looking into that.
Your code broke some code in the REST-tests, which means your code is ok, as that breakage was expected in case of making the change.

I've updated the server code in master c2a76f6412366703 accordingly.

Comment 5 Heiko W. Rupp 2013-09-03 14:46:31 UTC
Bulk closing of issues in old RHQ releases that are in production for a while now.

Please open a new issue when running into an issue.