Dangerous Code Example in EJB 3.1 Spec
I am currently working on a training concerning Java EE Best
Practices. For that reason I am reading through quite some material
about Java EE and EJB. However, looking at the EJB 3.1 spec I realized
that the code examples are an example of rather bad coding practice.
As an example here is the original code from EJB 3.1 Spec p. 348. It is supposed to
show how
UserTransactions can be used:
@Stateless
@TransactionManagement(BEAN)
public class MySessionBean implements MySession {
@Resource
javax.transaction.UserTransaction ut;
@Resource
javax.sql.DataSource database1;
@Resource
javax.sql.DataSource database2;
public void someMethod(...) {
java.sql.Connection con1;
java.sql.Connection con2;
java.sql.Statement stmt1;
java.sql.Statement stmt2;
// obtain con1 object and set it up for transactions
con1 = database1.getConnection();
stmt1 = con1.createStatement();
// obtain con2 object and set it up for transactions
con2 = database2.getConnection();
stmt2 = con2.createStatement();
//
// Now do a transaction that involves con1 and con2.
//
// start the transaction
ut.begin();
// Do some updates to both con1 and con2. The container
// automatically enlists con1 and con2 with the transaction. stmt1.executeQuery(...);
stmt1.executeUpdate(...);
stmt2.executeQuery(...);
stmt2.executeUpdate(...);
stmt1.executeUpdate(...);
stmt2.executeUpdate(...);
// commit the transaction
ut.commit();
// release connections
stmt1.close();
stmt2.close();
con1.close();
con2.close();
}
... }
What is wrong with this code? Well, first of all it does not
compile. Quite honestly I wouldn't really care too much about that - an
example is fine as long as the important point is still brought
across. But in this case it does matter as we will see later on. So here is
what the method should read like:
public void someMethod() throws SQLException, NotSupportedException,
SystemException, SecurityException, IllegalStateException,
RollbackException, HeuristicMixedException,
HeuristicRollbackException {
In the original example any code concerning Exceptions
has been left out. So what happens if an
Exceptions is thrown? No
close() is ever called and therefore none of the resources are ever cleaned up. Let's fix this:
@Stateless
@TransactionManagement(TransactionManagementType.BEAN)
public class MySessionBean {
@Resource
javax.transaction.UserTransaction ut;
@Resource
javax.sql.DataSource database1;
@Resource
javax.sql.DataSource database2;
public void someMethod() throws SQLException, NotSupportedException,
SystemException, SecurityException, IllegalStateException,
RollbackException, HeuristicMixedException,
HeuristicRollbackException {
java.sql.Connection con1 = null;
java.sql.Connection con2 = null;
java.sql.Statement stmt1 = null;
java.sql.Statement stmt2 = null;
try {
con1 = database1.getConnection();
stmt1 = con1.createStatement();
con2 = database2.getConnection();
stmt2 = con2.createStatement();
ut.begin();
stmt1.executeUpdate("");
stmt2.executeQuery("");
stmt2.executeUpdate("");
stmt1.executeUpdate("");
stmt2.executeUpdate("");
ut.commit();
} finally {
if (stmt2 != null)
try {
stmt2.close();
} catch (SQLException ex) {
}
if (con2 != null)
try {
con2.close();
} catch (SQLException ex) {
}
if (stmt1 != null)
try {
stmt1.close();
} catch (SQLException ex) {
}
if (con1 != null)
try {
con1.close();
} catch (SQLException ex) {
}
}
}
}
This is the infamous try-catch-finally-try-catch block. The
null
checks actually avoid calling
close() on objects that have not been
created - as the creation might already have failed and and exception might have been thrown. As the
close() operation might throw an exception this also needs to be handled. IMHO it is OK to swallow the exception - there is not really anything that can be done as the resources are already being closed. Using Spring's
JdbcTemplate would have avoided the problem as the resource handling
is done by the template then. I would strongly recommend it in code like this. It can be used independently from the other parts of Spring - e.g. Dependency Injection can still be done by Java EE. See
http://static.springsource.org/spring/docs/3.1.x/spring-framework-reference/html/jdbc.html#jdbc-core for details.
Now there is another thing that is still not OK - actually the most
important point. The
UserTransaction is never committed nor rolled back
if an exception occurs. Let's fix this, too:
@Stateless
@TransactionManagement(TransactionManagementType.BEAN)
public class MySessionBean {
@Resource
javax.transaction.UserTransaction ut;
@Resource
javax.sql.DataSource database1;
@Resource
javax.sql.DataSource database2;
public void someMethod() throws Exception {
java.sql.Connection con1 = null;
java.sql.Connection con2 = null;
java.sql.Statement stmt1 = null;
java.sql.Statement stmt2 = null;
try {
con1 = database1.getConnection();
stmt1 = con1.createStatement();
con2 = database2.getConnection();
stmt2 = con2.createStatement();
ut.begin();
stmt1.executeUpdate("");
stmt2.executeQuery("");
stmt2.executeUpdate("");
stmt1.executeUpdate("");
stmt2.executeUpdate("");
} catch (Exception ex) {
ut.setRollbackOnly();
throw ex;
} finally {
ut.commit();
if (stmt2 != null)
try {
stmt2.close();
} catch (SQLException ex) {
}
if (con2 != null)
try {
con2.close();
} catch (SQLException ex) {
}
if (stmt1 != null)
try {
stmt1.close();
} catch (SQLException ex) {
}
if (con1 != null)
try {
con1.close();
} catch (SQLException ex) {
}
}
}
}
So when a problem occurs the transaction will now be marked as
rollback only and ultimately rolled back. If there is no exception it will be
committed.
I hope I made no further mistakes in the code - let me know
otherwise. In Spring resource handling is done for you by the
Templates and therefore I might be doing something wrong here. Using
that approach would also have made the code a lot less complex.
So why this blog post? Bad resource handling and transaction
handling is far too common. I have done a lot of reviews and usually
Enterprise Java applications fail to do a good job in this
regard. This is dangerous, because
- The whole point of transactions
is to provide safety if something goes wrong. If you do not commit nor
roll back the transaction it will be left in an undefined state and all kinds of things might happen. This is
compromising the safety of transactions. So the transactions become
more or less
useless. You might as well just not use them at all.
- Not closing connections and statements will eventually lead to
resource leaks e.g. connection pools running out of connections. This
will at one point make the system fail.
- These problems are hard to detect - they will only surface if
exceptions are thrown. So finding and eliminating the problem is
complex. You should rather get it right from the start.
Sadly the EJB spec seem to do resource handling constantly wrong. The EJB spec is
not the only document that does resource and transaction handling
wrong. Quite the contrary: I have a read a lot of tutorial and other
documents that get this entirely wrong. However, IMHO at least the
example in the spec for using the
UserTransaction should show how to
use it in a bullet proof manner. This document is read by many
developers and other authors. Bad practices in such a document might
end up in a lot of code and other publications.
