Bug 55071 - try finally catch structure masks original exception in JDBCRealm.getPassword()
Summary: try finally catch structure masks original exception in JDBCRealm.getPassword()
Alias: None
Product: Tomcat 7
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 7.0.40
Hardware: Other All
: P2 normal (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
Depends on:
Reported: 2013-06-06 22:26 UTC by bruce weertman
Modified: 2013-06-20 21:48 UTC (History)
0 users


Note You need to log in before you can comment on or make changes to this bug.
Description bruce weertman 2013-06-06 22:26:48 UTC
I have run into a mysterious apparent SQL exception in JDBCRealm.getPassword().

Presumably, line 557 "stmt.executeQuery()" occasionally fails. I'm not sure why and would like to know why. 

However, the finally block contains dbConnection.commit() (line 579). This throws an exception (Connection Closed) which is then logged on line 585. 

In summary, line 579 should be surrounded by a try catch.
Comment 1 Christopher Schultz 2013-06-07 14:18:28 UTC
That commit() looks weird to me. COMMIT after SELECT?
Comment 2 bruce weertman 2013-06-07 16:03:53 UTC
I was wondering about that. I never do that on SELECT statements myself. After I read that piece of code, I was thinking that perhaps I've been doing SQL wrong for all these years! 

Looking at http://docs.oracle.com/javase/6/docs/api/java/sql/Connection.html#setAutoCommit%28%29 it sounds like rs.close() commits the select. 

So perhaps the solution is to just get rid of the commit() statement.
Comment 3 Christopher Schultz 2013-06-07 16:20:31 UTC
While this is likely to be fixed, you should understand that JDBCRealm is not appropriate for any real-world situation. Instead, you should use DataSourceRealm, which is much more robust.
Comment 4 bruce weertman 2013-06-07 18:52:48 UTC
Thanks I've made the switch.
Comment 5 Mark Thomas 2013-06-20 20:04:27 UTC
Ah, the joys of svn archeology.

The dbConnection.commit() is a result of fixing 10623. I am not convinced it was ever necessary. The fix for 10623 also added the rs.close() which does essentially the same thing.

This has been fixed in trunk and 7.0.x and will be included in 7.0.42 onwards.
Comment 6 Mark Thomas 2013-06-20 21:39:44 UTC
Opps. My analysis assumed autoCommit == true which it doesn't. The commit is therefore required. I've restored it and cleaned up the code.
Comment 7 Christopher Schultz 2013-06-20 21:48:05 UTC
I still think commit() should not be called. For the shared-connection of JDBCRealm to ever be put into auto-commit=false would be an enormous mistake, assuming any data modification was taking place... which it is not.