J and I and Me
2012-02-06
  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 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.
  16:33
Bookmark and Share
Comments:
I think the root cause is combining resource acquisition with transaction management.

Shame that the EJB spec or even the Java language does not deal with the resources management and opening. I'll have a think about a solution in Scala, if I can think of one.
 
Hi Jan,

thanks for your comment! The problem is solved in Java 7 with the new resource management, see http://www.oracle.com/technetwork/articles/java/trywithresources-401775.html .

Actually it is easy to solve the problem in Scala as well. You can use the template approach of Spring and combine it with higher order functions. See slide 20ff of http://www.oracle.com/technetwork/articles/java/trywithresources-401775.html .

Best regards,
Eberhard
 
I think in your last example the statements/connections won't be closed properly if ut.commit(); throws an exception.
 
Hi,
true. Should read:
} finally {
try {
ut.commit();
} catch (Exception ex) {
}
}
Thanks!
Eberhard
 
shouldn't be
ut.commit();
in the try-block, instead of the finally-block?
 
Good question. I don't think so as an exception would prevent commit() from being called. But you would need to check the status the Tx is in.

} finally {
try {
if (transaction.getStatus() != Status.STATUS_NO_TRANSACTION) {ut.commit();}
} catch (Exception ex) {
}
}
 
You're commit-ing it wrong.
The commit should be called before the "catch(Exception)" area. Why? Because you only want to commit if there are no exceptions (in case there are, you rollback anyway first, so committing will throw an exception anyway).
the standard pattern is
try {
begin transaction
do stuff()
commit transaction
} catch (exception(s)) {
rollback
} finally {
free resources
}
So yes, you're judging the spec guys, but you didn't test your code either. Or you tested it only for the "working" case
 
Hi,
thanks for your comment! Please note that the transaction will be rolled back if an exception occurs because the setRollbackOnly() is called in the catch block.
 
Hi,
I think an important aspect is still missing: the handling of ever-blocking operations. So a time-out should be added on every DB-operation (yes, we already got an endless blocking on close() operation using the Oracle JDBC-driver...) to prevent the blocking spread out into the whole application. This makes the code even more ugly and a template more valuable. Btw: does the Spring JDBC template handle time-outs out of the box? Or has it to be boiler-plately annotated every time?

Best regards,
Jens Nerche
 
Hi Eberhar,

Nice blog! Is there an email address I can contact you in private?
 
Sure - info springbuch.de
 
Kommentar veröffentlichen

<< Home
J for Java | I for Internet, iMac, iPod and iPad | Me for me

ARCHIVES
Juni 2005 / Juli 2005 / August 2005 / September 2005 / Oktober 2005 / November 2005 / Dezember 2005 / Januar 2006 / Februar 2006 / März 2006 / April 2006 / Mai 2006 / Juni 2006 / Juli 2006 / August 2006 / September 2006 / Oktober 2006 / November 2006 / Dezember 2006 / Januar 2007 / Februar 2007 / März 2007 / April 2007 / Mai 2007 / Juni 2007 / Juli 2007 / August 2007 / September 2007 / Oktober 2007 / November 2007 / Dezember 2007 / Januar 2008 / April 2008 / Mai 2008 / Juni 2008 / August 2008 / September 2008 / November 2008 / Januar 2009 / Februar 2009 / März 2009 / April 2009 / Mai 2009 / Juni 2009 / Juli 2009 / August 2009 / September 2009 / Oktober 2009 / November 2009 / Dezember 2009 / Januar 2010 / Februar 2010 / März 2010 / April 2010 / Mai 2010 / Juli 2010 / August 2010 / Oktober 2010 / Januar 2011 / Februar 2011 / März 2011 / April 2011 / Mai 2011 / Juni 2011 / August 2011 / September 2011 / November 2011 / Februar 2012 / April 2012 / Mai 2012 / April 2013 / Mai 2013 / Juni 2013 / Januar 2015 / Juli 2015 / Februar 2016 /

Links

Twitter
Google +
Slideshare
Prezi
XING
LinkedIn
Das Spring Buch


Feeds

Feedburner


Impressum
Betreiber und Kontakt:
Eberhard Wolff
Leobschützer Strasse 22
13125 Berlin
E-Mail-Adresse: eberhard.wolff@gmail.com

Verantwortlich für journalistisch-redaktionelle Inhalte:
Eberhard Wolff