From 8d9bc3afb15f52b6e62a3334b41ca5121db70660 Mon Sep 17 00:00:00 2001 From: Robert Gaggl Date: Mon, 8 Apr 2013 17:30:31 +0200 Subject: [PATCH 1/4] Next attempt to fix the deadlock issue that lead to fd0b77bc11f09106de0636de663bb3d16f7b02c9: The source of the deadlock problem seems that during DbSource.getConnection() ResourceProperties instances are compared using their equals() method, which is synchronized in Hashtable and can/does lead to deadlocks (see http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6582568). This commit overwrites equals with an unsynchronized version. Note that this implementation might return a wrong result if one of the two instances is modified during this method call, but at least doesn't throw a ConcurrentModificationException. --- src/helma/util/ResourceProperties.java | 49 ++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/src/helma/util/ResourceProperties.java b/src/helma/util/ResourceProperties.java index 9bc78063..07ba9d26 100644 --- a/src/helma/util/ResourceProperties.java +++ b/src/helma/util/ResourceProperties.java @@ -537,4 +537,53 @@ public class ResourceProperties extends Properties { super.clear(); } + /** + * Compares this ResourceProperties instance to the one passed + * as argument. Note that in contrast to Hashtable.equals this method + * isn't synchronized to avoid deadlocks (which can happen in eg. + * DbSource.equals), and the comparison might return a wrong result + * if one of the two instances is modified during this method call. This + * method however doesn't throw a ConcurrentModificationException. + * + * @param o object to be compared for equality with this instance + * @return true if the specified Object is equal to this instance + * @see Hashtable#equals(Object) + */ + public boolean equals(Object o) { + if (o == this) { + return true; + } + + if (!(o instanceof ResourceProperties)) { + return false; + } + ResourceProperties t = (ResourceProperties) o; + if (t.size() != size()) { + return false; + } + + try { + Object[] keys = keySet().toArray(); + for (Object key : keys) { + Object value = get(key); + if (value == null) { + if (!(t.get(key) == null && t.containsKey(key))) { + return false; + } + } else { + if (!value.equals(t.get(key))) { + return false; + } + } + } + } catch (ClassCastException unused) { + return false; + } catch (NullPointerException unused) { + return false; + } + + return true; + } + + } From 5f18e3ae2dd62dcd2d6a29840dc0b5ecce76ae07 Mon Sep 17 00:00:00 2001 From: Robert Gaggl Date: Tue, 9 Apr 2013 12:52:06 +0200 Subject: [PATCH 2/4] Modified Transactor to store sqlConnections internally using the name of the DbSource as Map key, not the DbSource instance. Using the instance as key is both inefficient and error prone (see fd0b77bc11f09106de0636de663bb3d16f7b02c9). Additional changes: - modified getConnection() to check if the DB is oracle. "SELECT 1" is invalid for Oracle DBs and lead to Helma dropping in-use connections every minute. - set DbSource name final --- src/helma/objectmodel/db/DbSource.java | 2 +- src/helma/objectmodel/db/Transactor.java | 24 ++++++++++++++---------- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/src/helma/objectmodel/db/DbSource.java b/src/helma/objectmodel/db/DbSource.java index 2e18179c..33e1dbc8 100644 --- a/src/helma/objectmodel/db/DbSource.java +++ b/src/helma/objectmodel/db/DbSource.java @@ -32,7 +32,7 @@ import java.util.Hashtable; public class DbSource { private static ResourceProperties defaultProps = null; private Properties conProps; - private String name; + private final String name; private ResourceProperties props, subProps; protected String url; private String driver; diff --git a/src/helma/objectmodel/db/Transactor.java b/src/helma/objectmodel/db/Transactor.java index ceb43043..4ebaa31c 100644 --- a/src/helma/objectmodel/db/Transactor.java +++ b/src/helma/objectmodel/db/Transactor.java @@ -52,10 +52,10 @@ public class Transactor { protected ITransaction txn; // Transactions for SQL data sources - private Map sqlConnections; + private Map sqlConnections; // Set of SQL connections that already have been verified - private Map testedConnections; + private Map testedConnections; // when did the current transaction start? private long tstart; @@ -81,8 +81,8 @@ public class Transactor { cleanNodes = new HashMap(); parentNodes = new HashSet(); - sqlConnections = new HashMap(); - testedConnections = new HashMap(); + sqlConnections = new HashMap(); + testedConnections = new HashMap(); active = false; killed = false; } @@ -238,9 +238,9 @@ public class Transactor { * @param con the connection */ public void registerConnection(DbSource src, Connection con) { - sqlConnections.put(src, con); + sqlConnections.put(src.getName(), con); // we assume a freshly created connection is ok. - testedConnections.put(src, new Long(System.currentTimeMillis())); + testedConnections.put(src.getName(), new Long(System.currentTimeMillis())); } /** @@ -249,16 +249,20 @@ public class Transactor { * @return the connection */ public Connection getConnection(DbSource src) { - Connection con = (Connection) sqlConnections.get(src); - Long tested = (Long) testedConnections.get(src); + Connection con = sqlConnections.get(src.getName()); + Long tested = testedConnections.get(src.getName()); long now = System.currentTimeMillis(); if (con != null && (tested == null || now - tested.longValue() > 60000)) { // Check if the connection is still alive by executing a simple statement. try { Statement stmt = con.createStatement(); - stmt.execute("SELECT 1"); + if (src.isOracle()) { + stmt.execute("SELECT 1 FROM DUAL"); + } else { + stmt.execute("SELECT 1"); + } stmt.close(); - testedConnections.put(src, new Long(now)); + testedConnections.put(src.getName(), new Long(now)); } catch (SQLException sx) { try { con.close(); From 61bafb72d6cb564f5a5197e7c173211f41befe0e Mon Sep 17 00:00:00 2001 From: Robert Gaggl Date: Tue, 9 Apr 2013 16:05:07 +0200 Subject: [PATCH 3/4] partly reverted 5f18e3ae2dd62dcd2d6a29840dc0b5ecce76ae07: switched back to using DbSources as Map keys, as using strings breaks switching databases using DbSource.switchProperties (which is used in jala.Test) --- src/helma/objectmodel/db/Transactor.java | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/helma/objectmodel/db/Transactor.java b/src/helma/objectmodel/db/Transactor.java index 4ebaa31c..ee6364ea 100644 --- a/src/helma/objectmodel/db/Transactor.java +++ b/src/helma/objectmodel/db/Transactor.java @@ -52,10 +52,10 @@ public class Transactor { protected ITransaction txn; // Transactions for SQL data sources - private Map sqlConnections; + private Map sqlConnections; // Set of SQL connections that already have been verified - private Map testedConnections; + private Map testedConnections; // when did the current transaction start? private long tstart; @@ -81,8 +81,8 @@ public class Transactor { cleanNodes = new HashMap(); parentNodes = new HashSet(); - sqlConnections = new HashMap(); - testedConnections = new HashMap(); + sqlConnections = new HashMap(); + testedConnections = new HashMap(); active = false; killed = false; } @@ -238,9 +238,9 @@ public class Transactor { * @param con the connection */ public void registerConnection(DbSource src, Connection con) { - sqlConnections.put(src.getName(), con); + sqlConnections.put(src, con); // we assume a freshly created connection is ok. - testedConnections.put(src.getName(), new Long(System.currentTimeMillis())); + testedConnections.put(src, new Long(System.currentTimeMillis())); } /** @@ -249,8 +249,8 @@ public class Transactor { * @return the connection */ public Connection getConnection(DbSource src) { - Connection con = sqlConnections.get(src.getName()); - Long tested = testedConnections.get(src.getName()); + Connection con = sqlConnections.get(src); + Long tested = testedConnections.get(src); long now = System.currentTimeMillis(); if (con != null && (tested == null || now - tested.longValue() > 60000)) { // Check if the connection is still alive by executing a simple statement. @@ -262,7 +262,7 @@ public class Transactor { stmt.execute("SELECT 1"); } stmt.close(); - testedConnections.put(src.getName(), new Long(now)); + testedConnections.put(src, new Long(now)); } catch (SQLException sx) { try { con.close(); From dd8f8b6caac5e51b090cdc157a79472c192101e8 Mon Sep 17 00:00:00 2001 From: Simon Oberhammer Date: Wed, 12 Mar 2014 09:56:29 +0100 Subject: [PATCH 4/4] bump version --- build.xml | 2 +- src/helma/main/Server.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/build.xml b/build.xml index 9c0d5886..15625912 100644 --- a/build.xml +++ b/build.xml @@ -8,7 +8,7 @@ - + diff --git a/src/helma/main/Server.java b/src/helma/main/Server.java index e9ed7309..32fb0a75 100644 --- a/src/helma/main/Server.java +++ b/src/helma/main/Server.java @@ -38,7 +38,7 @@ import helma.util.ResourceProperties; */ public class Server implements Runnable { // version string - public static final String version = "1.7.0 (__builddate__)"; + public static final String version = "1.7.3 (__builddate__)"; // static server instance private static Server server;