Menu
▾
▴
springframework-developer
|
From: Trevor C. <pr...@se...> - 2003-09-22 01:38:53
|
I'm currently rewriting some of the tests for the jdbc package, and I have
encountered a potential problem. Specifically, in the JdbcTemplate
"doWithResultSetFromPreparedQuery" method, exception handling does not
appear to always work. Recently changes were made which placed various
"close" calls in a catch (SQLException) block. However, if a Spring
exception is thrown (I noticed this by running a SqlFunction which returned
more than 1 row, thus throwing an "InvalidDataAccessApiUsageException") the
"SQLException" catch is NOT called, and the method exits without closing
resultset/preparedstatements, etc. Note that while I noticed this in the
single method, this behaviour permeates the JdbcTemplate.
The current ("paraphrased") code is:
<code>
try {
... various code ...
SQLWarning warning = ps.getWarnings();
rs.close();
ps.close();
throwExceptionOnWarningIfNotIgnoringWarnings(warning);
} catch (SQLException ex) {
if (rs != null) {
try {
rs.close();
} catch (SQLException ignore) {}
}
if (ps != null) {
try {
ps.close();
} catch (SQLException ignore) {}
}
throw getExceptionTranslator().translate("JdbcTemplate.query(psc) with
PreparedStatementCreator [" + psc + "]", null, ex);
} finally {
DataSourceUtils.closeConnectionIfNecessary(con, this.dataSource);
}
</code>
I propose changing it to:
<code>
try {
... various code ...
SQLWarning warning = ps.getWarnings();
throwExceptionOnWarningIfNotIgnoringWarnings(warning);
} catch (SQLException ex) {
throw getExceptionTranslator().translate("JdbcTemplate.query(psc) with
PreparedStatementCreator [" + psc + "]", null, ex);
} finally {
if (rs != null) {
try {
rs.close();
} catch (SQLException ignore) {}
}
if (ps != null) {
try {
ps.close();
} catch (SQLException ignore) {}
}
DataSourceUtils.closeConnectionIfNecessary(con, this.dataSource);
}
</code>
I believe this will ensure resources are closed regardless of
success/failure, and still allow Spring exceptions and "translated"
SQLExceptions to propogate as designed. If anyone sees any holes in this
(specifically Rod/Thomas who I believe have done the most work on the jdbc
packages) please let me know. Otherwise (and barring exceptions) I will
make the changes and commit them with the updated tests tomorrow.
Trevor D. Cook
|
|
From: Rod J. <rod...@in...> - 2003-09-22 07:11:07
|
Trevor
I think you're right, although I wasn't responsible for the current error
handling and am probably not best placed to comment.
I guess the first thing is break the current code with more rigorous tests,
which you seem to have done, before trying any changes.
Regards,
Rod
----- Original Message -----
From: "Trevor Cook" <pr...@se...>
To: "Spring Developers" <spr...@li...>
Sent: Monday, September 22, 2003 2:39 AM
Subject: [Springframework-developer] potential JdbcTemplate bug
> I'm currently rewriting some of the tests for the jdbc package, and I have
> encountered a potential problem. Specifically, in the JdbcTemplate
> "doWithResultSetFromPreparedQuery" method, exception handling does not
> appear to always work. Recently changes were made which placed various
> "close" calls in a catch (SQLException) block. However, if a Spring
> exception is thrown (I noticed this by running a SqlFunction which
returned
> more than 1 row, thus throwing an "InvalidDataAccessApiUsageException")
the
> "SQLException" catch is NOT called, and the method exits without closing
> resultset/preparedstatements, etc. Note that while I noticed this in the
> single method, this behaviour permeates the JdbcTemplate.
>
> The current ("paraphrased") code is:
>
> <code>
> try {
> ... various code ...
> SQLWarning warning = ps.getWarnings();
> rs.close();
> ps.close();
> throwExceptionOnWarningIfNotIgnoringWarnings(warning);
> } catch (SQLException ex) {
> if (rs != null) {
> try {
> rs.close();
> } catch (SQLException ignore) {}
> }
> if (ps != null) {
> try {
> ps.close();
> } catch (SQLException ignore) {}
> }
> throw getExceptionTranslator().translate("JdbcTemplate.query(psc) with
> PreparedStatementCreator [" + psc + "]", null, ex);
> } finally {
> DataSourceUtils.closeConnectionIfNecessary(con, this.dataSource);
> }
> </code>
>
> I propose changing it to:
>
> <code>
> try {
> ... various code ...
> SQLWarning warning = ps.getWarnings();
> throwExceptionOnWarningIfNotIgnoringWarnings(warning);
> } catch (SQLException ex) {
> throw getExceptionTranslator().translate("JdbcTemplate.query(psc) with
> PreparedStatementCreator [" + psc + "]", null, ex);
> } finally {
> if (rs != null) {
> try {
> rs.close();
> } catch (SQLException ignore) {}
> }
> if (ps != null) {
> try {
> ps.close();
> } catch (SQLException ignore) {}
> }
> DataSourceUtils.closeConnectionIfNecessary(con, this.dataSource);
> }
> </code>
>
> I believe this will ensure resources are closed regardless of
> success/failure, and still allow Spring exceptions and "translated"
> SQLExceptions to propogate as designed. If anyone sees any holes in this
> (specifically Rod/Thomas who I believe have done the most work on the jdbc
> packages) please let me know. Otherwise (and barring exceptions) I will
> make the changes and commit them with the updated tests tomorrow.
>
> Trevor D. Cook
>
>
>
> -------------------------------------------------------
> This sf.net email is sponsored by:ThinkGeek
> Welcome to geek heaven.
> http://thinkgeek.com/sf
> _______________________________________________
> Springframework-developer mailing list
> Spr...@li...
> https://lists.sourceforge.net/lists/listinfo/springframework-developer
|
|
From: <tri...@tr...> - 2003-09-23 01:36:38
|
Rod & Trevor,
After a closer look I'm convince that Trevor is right - the finally block is the
safest place to close all resources. Did not consider non SQLExceptions, but as
you point out, they would not be caught and we could leave resultsets open.
Thomas
> Trevor
>
> I think you're right, although I wasn't responsible for the current error
> handling and am probably not best placed to comment.
>
> I guess the first thing is break the current code with more rigorous tests,
> which you seem to have done, before trying any changes.
>
> Regards,
> Rod
>
> ----- Original Message -----
> From: "Trevor Cook" <pr...@se...>
> To: "Spring Developers" <spr...@li...>
> Sent: Monday, September 22, 2003 2:39 AM
> Subject: [Springframework-developer] potential JdbcTemplate bug
>
>
> > I'm currently rewriting some of the tests for the jdbc package, and I have
> > encountered a potential problem. Specifically, in the JdbcTemplate
> > "doWithResultSetFromPreparedQuery" method, exception handling does not
> > appear to always work. Recently changes were made which placed various
> > "close" calls in a catch (SQLException) block. However, if a Spring
> > exception is thrown (I noticed this by running a SqlFunction which
> returned
> > more than 1 row, thus throwing an "InvalidDataAccessApiUsageException")
> the
> > "SQLException" catch is NOT called, and the method exits without closing
> > resultset/preparedstatements, etc. Note that while I noticed this in the
> > single method, this behaviour permeates the JdbcTemplate.
> >
> > The current ("paraphrased") code is:
> >
> > <code>
> > try {
> > ... various code ...
> > SQLWarning warning = ps.getWarnings();
> > rs.close();
> > ps.close();
> > throwExceptionOnWarningIfNotIgnoringWarnings(warning);
> > } catch (SQLException ex) {
> > if (rs != null) {
> > try {
> > rs.close();
> > } catch (SQLException ignore) {}
> > }
> > if (ps != null) {
> > try {
> > ps.close();
> > } catch (SQLException ignore) {}
> > }
> > throw getExceptionTranslator().translate("JdbcTemplate.query(psc) with
> > PreparedStatementCreator [" + psc + "]", null, ex);
> > } finally {
> > DataSourceUtils.closeConnectionIfNecessary(con, this.dataSource);
> > }
> > </code>
> >
> > I propose changing it to:
> >
> > <code>
> > try {
> > ... various code ...
> > SQLWarning warning = ps.getWarnings();
> > throwExceptionOnWarningIfNotIgnoringWarnings(warning);
> > } catch (SQLException ex) {
> > throw getExceptionTranslator().translate("JdbcTemplate.query(psc) with
> > PreparedStatementCreator [" + psc + "]", null, ex);
> > } finally {
> > if (rs != null) {
> > try {
> > rs.close();
> > } catch (SQLException ignore) {}
> > }
> > if (ps != null) {
> > try {
> > ps.close();
> > } catch (SQLException ignore) {}
> > }
> > DataSourceUtils.closeConnectionIfNecessary(con, this.dataSource);
> > }
> > </code>
> >
> > I believe this will ensure resources are closed regardless of
> > success/failure, and still allow Spring exceptions and "translated"
> > SQLExceptions to propogate as designed. If anyone sees any holes in this
> > (specifically Rod/Thomas who I believe have done the most work on the jdbc
> > packages) please let me know. Otherwise (and barring exceptions) I will
> > make the changes and commit them with the updated tests tomorrow.
> >
> > Trevor D. Cook
> >
> >
> >
> > -------------------------------------------------------
> > This sf.net email is sponsored by:ThinkGeek
> > Welcome to geek heaven.
> > http://thinkgeek.com/sf
> > _______________________________________________
> > Springframework-developer mailing list
> > Spr...@li...
> > https://lists.sourceforge.net/lists/listinfo/springframework-developer
>
>
>
> -------------------------------------------------------
> This sf.net email is sponsored by:ThinkGeek
> Welcome to geek heaven.
> http://thinkgeek.com/sf
> _______________________________________________
> Springframework-developer mailing list
> Spr...@li...
> https://lists.sourceforge.net/lists/listinfo/springframework-developer
>
|
