You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@cocoon.apache.org by an...@apache.org on 2005/01/08 03:53:57 UTC
svn commit: r124610 - in cocoon/branches/BRANCH_2_1_X/src/blocks: databases/java/org/apache/cocoon/acting databases/java/org/apache/cocoon/components/source/impl databases/java/org/apache/cocoon/reading hsqldb/java/org/apache/cocoon/components/hsqldb petstore/java/org/apache/cocoon/components/flow/javascript
Author: antonio
Date: Fri Jan 7 18:53:56 2005
New Revision: 124610
URL: http://svn.apache.org/viewcvs?view=rev&rev=124610
Log:
Close database resource
Modified:
cocoon/branches/BRANCH_2_1_X/src/blocks/databases/java/org/apache/cocoon/acting/OraAddAction.java
cocoon/branches/BRANCH_2_1_X/src/blocks/databases/java/org/apache/cocoon/components/source/impl/BlobSource.java
cocoon/branches/BRANCH_2_1_X/src/blocks/databases/java/org/apache/cocoon/reading/DatabaseReader.java
cocoon/branches/BRANCH_2_1_X/src/blocks/hsqldb/java/org/apache/cocoon/components/hsqldb/ServerImpl.java
cocoon/branches/BRANCH_2_1_X/src/blocks/petstore/java/org/apache/cocoon/components/flow/javascript/ScriptableConnection.java
Modified: cocoon/branches/BRANCH_2_1_X/src/blocks/databases/java/org/apache/cocoon/acting/OraAddAction.java
Url: http://svn.apache.org/viewcvs/cocoon/branches/BRANCH_2_1_X/src/blocks/databases/java/org/apache/cocoon/acting/OraAddAction.java?view=diff&rev=124610&p1=cocoon/branches/BRANCH_2_1_X/src/blocks/databases/java/org/apache/cocoon/acting/OraAddAction.java&r1=124609&p2=cocoon/branches/BRANCH_2_1_X/src/blocks/databases/java/org/apache/cocoon/acting/OraAddAction.java&r2=124610
==============================================================================
--- cocoon/branches/BRANCH_2_1_X/src/blocks/databases/java/org/apache/cocoon/acting/OraAddAction.java (original)
+++ cocoon/branches/BRANCH_2_1_X/src/blocks/databases/java/org/apache/cocoon/acting/OraAddAction.java Fri Jan 7 18:53:56 2005
@@ -51,7 +51,7 @@
* only one table at a time to update.
*
* @author <a href="mailto:bloritsch@apache.org">Berin Loritsch</a>
- * @version CVS $Id: OraAddAction.java,v 1.4 2004/03/30 05:50:48 antonio Exp $
+ * @version CVS $Id$
*/
public class OraAddAction extends DatabaseAddAction {
private static final Map selectLOBStatements = new HashMap();
@@ -91,18 +91,23 @@
if ("manual".equals(mode)) {
String selectQuery = this.getSelectQuery(keys[i]);
-
- ResultSet set = conn.createStatement().executeQuery(selectQuery);
- set.next();
- int value = set.getInt("maxid") + 1;
-
- statement.setInt(currentIndex, value);
-
- request.setAttribute(keys[i].getAttribute("param"), String.valueOf(value));
-
- set.close();
- set.getStatement().close();
- currentIndex++;
+ ResultSet set = null;
+ try {
+ set = conn.createStatement().executeQuery(selectQuery);
+ set.next();
+ int value = set.getInt("maxid") + 1;
+
+ statement.setInt(currentIndex, value);
+
+ request.setAttribute(keys[i].getAttribute("param"), String.valueOf(value));
+ } catch (SQLException sqle){
+ getLogger().warn("There was an error closing the ResultSet", sqle);
+ throw sqle;
+ } finally {
+ set.close();
+ set.getStatement().close();
+ currentIndex++;
+ }
} else if ("form".equals(mode)) {
String parameter = keys[i].getAttribute("param");
request.setAttribute(parameter, request.getParameter(parameter));
@@ -229,8 +234,9 @@
getLogger().warn("There was an error closing the datasource", sqe);
}
}
-
- if (datasource != null) this.dbselector.release(datasource);
+ if (datasource != null) {
+ this.dbselector.release(datasource);
+ }
}
return null;
Modified: cocoon/branches/BRANCH_2_1_X/src/blocks/databases/java/org/apache/cocoon/components/source/impl/BlobSource.java
Url: http://svn.apache.org/viewcvs/cocoon/branches/BRANCH_2_1_X/src/blocks/databases/java/org/apache/cocoon/components/source/impl/BlobSource.java?view=diff&rev=124610&p1=cocoon/branches/BRANCH_2_1_X/src/blocks/databases/java/org/apache/cocoon/components/source/impl/BlobSource.java&r1=124609&p2=cocoon/branches/BRANCH_2_1_X/src/blocks/databases/java/org/apache/cocoon/components/source/impl/BlobSource.java&r2=124610
==============================================================================
--- cocoon/branches/BRANCH_2_1_X/src/blocks/databases/java/org/apache/cocoon/components/source/impl/BlobSource.java (original)
+++ cocoon/branches/BRANCH_2_1_X/src/blocks/databases/java/org/apache/cocoon/components/source/impl/BlobSource.java Fri Jan 7 18:53:56 2005
@@ -60,7 +60,7 @@
*
* @author <a href="mailto:sylvain@apache.org">Sylvain Wallez</a>
* @author <a href="mailto:stephan@apache.org">Stephan Michels</a>
- * @version CVS $Id: BlobSource.java,v 1.6 2004/03/11 15:34:30 sylvain Exp $
+ * @version CVS $Id$
*/
public class BlobSource extends AbstractLogEnabled implements Source, Serviceable {
@@ -201,16 +201,18 @@
default :
String value = rs.getString(1);
- stmt.close();
- rs.close();
- cnx.close();
return new ByteArrayInputStream(value.getBytes());
}
} catch(SQLException sqle) {
String msg = "Cannot retrieve content from " + this.systemId;
getLogger().error(msg, sqle);
-
+ // IOException would be more adequate, but ProcessingException is cascaded...
+ throw new SourceException(msg, sqle);
+ } finally {
try {
+ if (stmt != null) {
+ stmt.close();
+ }
if (cnx != null) {
cnx.close();
}
@@ -218,9 +220,6 @@
// PITA
throw new SourceException("Cannot close connection", sqle2);
}
-
- // IOException would be more adequate, but ProcessingException is cascaded...
- throw new SourceException(msg, sqle);
}
}
Modified: cocoon/branches/BRANCH_2_1_X/src/blocks/databases/java/org/apache/cocoon/reading/DatabaseReader.java
Url: http://svn.apache.org/viewcvs/cocoon/branches/BRANCH_2_1_X/src/blocks/databases/java/org/apache/cocoon/reading/DatabaseReader.java?view=diff&rev=124610&p1=cocoon/branches/BRANCH_2_1_X/src/blocks/databases/java/org/apache/cocoon/reading/DatabaseReader.java&r1=124609&p2=cocoon/branches/BRANCH_2_1_X/src/blocks/databases/java/org/apache/cocoon/reading/DatabaseReader.java&r2=124610
==============================================================================
--- cocoon/branches/BRANCH_2_1_X/src/blocks/databases/java/org/apache/cocoon/reading/DatabaseReader.java (original)
+++ cocoon/branches/BRANCH_2_1_X/src/blocks/databases/java/org/apache/cocoon/reading/DatabaseReader.java Fri Jan 7 18:53:56 2005
@@ -52,7 +52,7 @@
* to pull the image from, and source specifies the source key information.
*
* @author <a href="mailto:bloritsch@apache.org">Berin Loritsch</a>
- * @version CVS $Id: DatabaseReader.java,v 1.5 2004/03/05 13:01:55 bdelacretaz Exp $
+ * @version CVS $Id$
*/
public class DatabaseReader extends ServiceableReader
implements Configurable, Disposable, CacheableProcessingComponent
@@ -91,6 +91,7 @@
throws ProcessingException, SAXException, IOException {
super.setup(resolver, objectModel, src, par);
+ PreparedStatement statement = null;
try {
this.datasource = (DataSourceComponent) dbselector.select(dsn);
this.con = datasource.getConnection();
@@ -99,7 +100,7 @@
this.con.setAutoCommit(false);
}
- PreparedStatement statement = con.prepareStatement(getQuery());
+ statement = con.prepareStatement(getQuery());
statement.setString(1, this.source);
ResultSet set = statement.executeQuery();
if (!set.next()) throw new ResourceNotFoundException("There is no resource with that key");
@@ -121,8 +122,15 @@
this.doCommit = true;
} catch (Exception e) {
this.doCommit = false;
-
throw new ResourceNotFoundException("DatabaseReader error:", e);
+ } finally {
+ try {
+ if (statement != null) {
+ statement.close();
+ }
+ } catch(SQLException sqle) {
+ throw new ResourceNotFoundException("Cannot close connection", sqle);
+ }
}
}
Modified: cocoon/branches/BRANCH_2_1_X/src/blocks/hsqldb/java/org/apache/cocoon/components/hsqldb/ServerImpl.java
Url: http://svn.apache.org/viewcvs/cocoon/branches/BRANCH_2_1_X/src/blocks/hsqldb/java/org/apache/cocoon/components/hsqldb/ServerImpl.java?view=diff&rev=124610&p1=cocoon/branches/BRANCH_2_1_X/src/blocks/hsqldb/java/org/apache/cocoon/components/hsqldb/ServerImpl.java&r1=124609&p2=cocoon/branches/BRANCH_2_1_X/src/blocks/hsqldb/java/org/apache/cocoon/components/hsqldb/ServerImpl.java&r2=124610
==============================================================================
--- cocoon/branches/BRANCH_2_1_X/src/blocks/hsqldb/java/org/apache/cocoon/components/hsqldb/ServerImpl.java (original)
+++ cocoon/branches/BRANCH_2_1_X/src/blocks/hsqldb/java/org/apache/cocoon/components/hsqldb/ServerImpl.java Fri Jan 7 18:53:56 2005
@@ -164,10 +164,11 @@
/** Stop the server */
public void stop() {
if (started) {
+ Statement statement = null;
try {
getLogger().debug("Shutting down HSQLDB");
Connection connection = DriverManager.getConnection("jdbc:hsqldb:hsql://localhost:" + this.port, "sa", "");
- Statement statement = connection.createStatement();
+ statement = connection.createStatement();
statement.executeQuery("SHUTDOWN");
try {
connection.close();
@@ -178,6 +179,14 @@
}
} catch (Exception e){
getLogger().error("Error while shutting down HSQLDB", e);
+ } finally {
+ try {
+ if (statement != null) {
+ statement.close();
+ }
+ } catch(SQLException sqle) {
+ getLogger().error("Error while shutting down HSQLDB", sqle);
+ }
}
getLogger().debug("Shutting down HSQLDB: Done");
}
Modified: cocoon/branches/BRANCH_2_1_X/src/blocks/petstore/java/org/apache/cocoon/components/flow/javascript/ScriptableConnection.java
Url: http://svn.apache.org/viewcvs/cocoon/branches/BRANCH_2_1_X/src/blocks/petstore/java/org/apache/cocoon/components/flow/javascript/ScriptableConnection.java?view=diff&rev=124610&p1=cocoon/branches/BRANCH_2_1_X/src/blocks/petstore/java/org/apache/cocoon/components/flow/javascript/ScriptableConnection.java&r1=124609&p2=cocoon/branches/BRANCH_2_1_X/src/blocks/petstore/java/org/apache/cocoon/components/flow/javascript/ScriptableConnection.java&r2=124610
==============================================================================
--- cocoon/branches/BRANCH_2_1_X/src/blocks/petstore/java/org/apache/cocoon/components/flow/javascript/ScriptableConnection.java (original)
+++ cocoon/branches/BRANCH_2_1_X/src/blocks/petstore/java/org/apache/cocoon/components/flow/javascript/ScriptableConnection.java Fri Jan 7 18:53:56 2005
@@ -18,6 +18,7 @@
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.ResultSetMetaData;
+import java.sql.SQLException;
import org.mozilla.javascript.Context;
import org.mozilla.javascript.Function;
@@ -66,7 +67,7 @@
* A ScriptableConnection is also a wrapper around a real JDBC Connection and thus
* provides all of methods of Connection as well
*
- * @version CVS $Id: ScriptableConnection.java,v 1.6 2004/03/05 13:02:03 bdelacretaz Exp $
+ * @version CVS $Id$
*/
public class ScriptableConnection extends ScriptableObject {
@@ -206,9 +207,11 @@
public Object jsFunction_query(String sql, Object params,
int startRow, int maxRows,
Object funObj)
- throws JavaScriptException {
+ throws JavaScriptException {
+
+ PreparedStatement stmt = null;
try {
- PreparedStatement stmt = connection.prepareStatement(sql);
+ stmt = connection.prepareStatement(sql);
Scriptable array = (Scriptable)params;
if (array != Undefined.instance) {
int len = (int)
@@ -272,13 +275,22 @@
throw e;
} catch (Exception e) {
throw new JavaScriptException(e);
+ } finally {
+ try {
+ if (stmt != null) {
+ stmt.close();
+ }
+ } catch (SQLException sqle) {
+ throw new JavaScriptException(sqle);
+ }
}
}
public int jsFunction_update(String sql, Object params)
throws JavaScriptException {
+ PreparedStatement stmt = null;
try {
- PreparedStatement stmt = connection.prepareStatement(sql);
+ stmt = connection.prepareStatement(sql);
Scriptable array = (Scriptable)params;
if (array != Undefined.instance) {
int len = (int)
@@ -298,6 +310,14 @@
return stmt.getUpdateCount();
} catch (Exception e) {
throw new JavaScriptException(e);
+ } finally {
+ try {
+ if (stmt != null) {
+ stmt.close();
+ }
+ } catch (SQLException sqle) {
+ throw new JavaScriptException(sqle);
+ }
}
}
Re: svn commit: r124610 - in
petstore/java/org/apache/cocoon/components/flow/javascript/...
Posted by Gunnar Brand <g....@interface-projects.de>.
I noticed (kinda late), that this is almost the same for the
ScriptableConnection as (the still open?) bug 30326
http://issues.apache.org/bugzilla/show_bug.cgi?id=30326
which was never applied imho. The patch proposed there discards the
exception in the try { close() } block.
The one I see here (Cocoon 2.1.7, svn web doesn't work at the time of this
writing, so I can't check head) does throw the exception caught for close.
I think both versions are not 100% perfect.
30326 discards exceptions happening when close() bugs out but the previous
statement worked fine. This is not good.
But it works fine if the statement failed by not throwing another exception
in finally, shadowing the first exception.
For example the statement failed with a good reason, and the close fails
with a not so good reason due to the statement failing. The developer will
have a hard time finding the real reason.
The current 2.1.7 version works the opposite way.
Good if the statement worked but close() fails and bad if the statement and
close() fail.
The only proper way I can think of would be storing the first thrown
exception in all catch blocks in a method wide variable and throwing it
wrapped into a javascript exception at the end of the finally block (i.e.
store and rethrow only if it is !=null).
Alternatively a flag indicating that a excpetion has been thrown works
quite well, too. Since this can be an exception, short ifs can be used to
avoid extra code.
I added some pseudo code to the quoted diff below for both ideas...
Gunnar
> ---
cocoon/branches/BRANCH_2_1_X/src/blocks/petstore/java/org/apache/cocoon/components
\
> /flow/javascript/ScriptableConnection.java (original)
> +++
cocoon/branches/BRANCH_2_1_X/src/blocks/petstore/java/org/apache/cocoon/components
\
> /flow/javascript/ScriptableConnection.java Fri Jan 7 18:53:56 2005
@@ -18,6 +18,7 @@
> import java.sql.PreparedStatement;
> import java.sql.ResultSet;
> import java.sql.ResultSetMetaData;
> +import java.sql.SQLException;
>
> import org.mozilla.javascript.Context;
> import org.mozilla.javascript.Function;
> @@ -66,7 +67,7 @@
> * A ScriptableConnection is also a wrapper around a real JDBC
Connection and thus
> * provides all of methods of Connection as well
> *
> - * @version CVS $Id: ScriptableConnection.java,v 1.6 2004/03/05
13:02:03 bdelacretaz \
> Exp $ + * @version CVS $Id$
> */
> public class ScriptableConnection extends ScriptableObject {
>
> @@ -206,9 +207,11 @@
> public Object jsFunction_query(String sql, Object params,
> int startRow, int maxRows,
> Object funObj)
> - throws JavaScriptException {
> + throws JavaScriptException {
> +
Exception firstException = null;
> + PreparedStatement stmt = null;
> try {
> - PreparedStatement stmt = connection.prepareStatement(sql);
> + stmt = connection.prepareStatement(sql);
> Scriptable array = (Scriptable)params;
> if (array != Undefined.instance) {
> int len = (int)
> @@ -272,13 +275,22 @@
> throw e;
> } catch (Exception e) {
> throw new JavaScriptException(e);
// ^^^ remove
firstException = e;
> + } finally {
> + try {
> + if (stmt != null) {
> + stmt.close();
> + }
> + } catch (SQLException sqle) {
> + throw new JavaScriptException(sqle);
// ^^^ remove
if ( firstException!=null ) firstException = sqle;
> + }
if ( firstException!=null ) throw new
JavaScriptException(firstException);
> }
> }
>
The next method needs these changes, too...
> public int jsFunction_update(String sql, Object params)
> throws JavaScriptException {
Exception thrownException = null;
> + PreparedStatement stmt = null;
> try {
> - PreparedStatement stmt = connection.prepareStatement(sql);
> + stmt = connection.prepareStatement(sql);
> Scriptable array = (Scriptable)params;
> if (array != Undefined.instance) {
> int len = (int)
> @@ -298,6 +310,14 @@
> return stmt.getUpdateCount();
> } catch (Exception e) {
> throw new JavaScriptException(e);
// ^^^ remove
throw new JavaScriptException(thrownException = e); //
first throw, no check necessary
> + } finally {
> + try {
> + if (stmt != null) {
> + stmt.close();
> + }
> + } catch (SQLException sqle) {
> + throw new JavaScriptException(sqle);
// ^^^ remove
> + throw new JavaScriptException(thrownException =
thrownException!=null ? thrownException : sqle);
> + }
> }
> }
>
--
G. Brand - interface projects GmbH
Tolkewitzer Strasse 49
D-01277 Dresden
mail: g.brand@interface-projects.de
tel: ++49-351-3 18 09 - 41
Ein Unternehmen der interface:business-Gruppe
Re: svn commit: r124610 - in
cocoon/branches/BRANCH_2_1_X/src/blocks:
databases/java/org/apache/cocoon/acting
databases/java/org/apache/cocoon/components/source/impl
databases/java/org/apache/cocoon/reading
hsqldb/java/org/apache/cocoon/components/hsqldb
petstore/java/org/apache/cocoon/components/flow/javascript
Posted by Antonio Gallardo <ag...@agssa.net>.
On Sab, 8 de Enero de 2005, 7:41, Ugo Cei dijo:
>> + ResultSet set = null;
>> + try {
>> + set =
>> conn.createStatement().executeQuery(selectQuery);
>
> Assuming the preceding statement throws an exception ...
>
>> + set.next();
>> + int value = set.getInt("maxid") + 1;
>> +
>> + statement.setInt(currentIndex, value);
>> +
>> +
>> request.setAttribute(keys[i].getAttribute("param"),
>> String.valueOf(value));
>> + } catch (SQLException sqle){
>> + getLogger().warn("There was an error closing
>> the ResultSet", sqle);
>> + throw sqle;
>> + } finally {
>> + set.close();
>> + set.getStatement().close();
>
> ... one of these last two statements will thrown a NPE, if I'm not
> mistaken. We should check that set and set.getStatement() are not null,
> right?
>
> Moreover:
>
>> + getLogger().warn("There was an error closing
>> the ResultSet", sqle);
>
> The exception is not caught while closing the ResultSet, so the message
> is misleading.
Thanks Ugo. You are right, I tried to fix it in a second commit:
http://marc.theaimsgroup.com/?l=xml-cocoon-cvs&m=110515609528180&w=2
But still need refactoring. I will review it now.
Best Regards,
Antonio Gallardo.
Re: svn commit: r124610 - in cocoon/branches/BRANCH_2_1_X/src/blocks: databases/java/org/apache/cocoon/acting databases/java/org/apache/cocoon/components/source/impl databases/java/org/apache/cocoon/reading hsqldb/java/org/apache/cocoon/components/hsqldb petstore/java/org/apache/cocoon/components/flow/javascript
Posted by Ugo Cei <ug...@apache.org>.
> + ResultSet set = null;
> + try {
> + set =
> conn.createStatement().executeQuery(selectQuery);
Assuming the preceding statement throws an exception ...
> + set.next();
> + int value = set.getInt("maxid") + 1;
> +
> + statement.setInt(currentIndex, value);
> +
> +
> request.setAttribute(keys[i].getAttribute("param"),
> String.valueOf(value));
> + } catch (SQLException sqle){
> + getLogger().warn("There was an error closing
> the ResultSet", sqle);
> + throw sqle;
> + } finally {
> + set.close();
> + set.getStatement().close();
... one of these last two statements will thrown a NPE, if I'm not
mistaken. We should check that set and set.getStatement() are not null,
right?
Moreover:
> + getLogger().warn("There was an error closing
> the ResultSet", sqle);
The exception is not caught while closing the ResultSet, so the message
is misleading.
Ugo
--
Ugo Cei - http://beblogging.com/blojsom/blog/