Repository: calcite-avatica Updated Branches: refs/heads/master d19740921 -> fcb1342a4
[CALCITE-2003] Remove global synchronization on openConnection Try to fix some TravisCI issues that have recently cropped up. Closes apache/calcite-avatica#17 Project: http://git-wip-us.apache.org/repos/asf/calcite-avatica/repo Commit: http://git-wip-us.apache.org/repos/asf/calcite-avatica/commit/fcb1342a Tree: http://git-wip-us.apache.org/repos/asf/calcite-avatica/tree/fcb1342a Diff: http://git-wip-us.apache.org/repos/asf/calcite-avatica/diff/fcb1342a Branch: refs/heads/master Commit: fcb1342a404a38b14ddefb1f4d81fd2c4c953718 Parents: d197409 Author: Josh Elser <[email protected]> Authored: Fri Oct 6 13:27:46 2017 -0400 Committer: Josh Elser <[email protected]> Committed: Mon Oct 9 12:02:29 2017 -0400 ---------------------------------------------------------------------- .travis.yml | 10 ++-- .../apache/calcite/avatica/jdbc/JdbcMeta.java | 35 +++++++++---- .../calcite/avatica/jdbc/JdbcMetaTest.java | 54 ++++++++++++++++++++ 3 files changed, 87 insertions(+), 12 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/calcite-avatica/blob/fcb1342a/.travis.yml ---------------------------------------------------------------------- diff --git a/.travis.yml b/.travis.yml index d54d884..034f285 100644 --- a/.travis.yml +++ b/.travis.yml @@ -17,9 +17,11 @@ # limitations under the License. # language: java -jdk: - - oraclejdk8 - - oraclejdk7 +matrix: + fast_finish: true + include: + - jdk: oraclejdk8 + - jdk: openjdk7 branches: only: - master @@ -32,6 +34,7 @@ install: - mvn -V install -DskipTests -Dmaven.javadoc.skip=true script: # Print surefire output to the console instead of files + - unset _JAVA_OPTIONS - mvn -Dsurefire.useFile=false verify git: depth: 10000 @@ -39,4 +42,5 @@ sudo: false cache: directories: - $HOME/.m2 +sudo: required # End .travis.yml http://git-wip-us.apache.org/repos/asf/calcite-avatica/blob/fcb1342a/server/src/main/java/org/apache/calcite/avatica/jdbc/JdbcMeta.java ---------------------------------------------------------------------- diff --git a/server/src/main/java/org/apache/calcite/avatica/jdbc/JdbcMeta.java b/server/src/main/java/org/apache/calcite/avatica/jdbc/JdbcMeta.java index 348d48f..38f92ff 100644 --- a/server/src/main/java/org/apache/calcite/avatica/jdbc/JdbcMeta.java +++ b/server/src/main/java/org/apache/calcite/avatica/jdbc/JdbcMeta.java @@ -67,6 +67,7 @@ import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Properties; +import java.util.concurrent.ConcurrentMap; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; @@ -216,6 +217,11 @@ public class JdbcMeta implements ProtobufMeta { } // For testing purposes + protected Cache<String, Connection> getConnectionCache() { + return connectionCache; + } + + // For testing purposes protected Cache<Integer, StatementInfo> getStatementCache() { return statementCache; } @@ -610,19 +616,30 @@ public class JdbcMeta implements ProtobufMeta { fullInfo.putAll(info); } - synchronized (this) { - try { - if (connectionCache.asMap().containsKey(ch.id)) { - throw new RuntimeException("Connection already exists: " + ch.id); - } - Connection conn = DriverManager.getConnection(url, fullInfo); - connectionCache.put(ch.id, conn); - } catch (SQLException e) { - throw new RuntimeException(e); + final ConcurrentMap<String, Connection> cacheAsMap = connectionCache.asMap(); + if (cacheAsMap.containsKey(ch.id)) { + throw new RuntimeException("Connection already exists: " + ch.id); + } + // Avoid global synchronization of connection opening + try { + Connection conn = createConnection(url, fullInfo); + Connection loadedConn = cacheAsMap.putIfAbsent(ch.id, conn); + // Race condition: someone beat us to storing the connection in the cache. + if (loadedConn != null) { + conn.close(); + throw new RuntimeException("Connection already exists: " + ch.id); } + } catch (SQLException e) { + throw new RuntimeException(e); } } + // Visible for testing + protected Connection createConnection(String url, Properties info) throws SQLException { + // Allows simpler testing of openConnection + return DriverManager.getConnection(url, info); + } + @Override public void closeConnection(ConnectionHandle ch) { Connection conn = connectionCache.getIfPresent(ch.id); if (conn == null) { http://git-wip-us.apache.org/repos/asf/calcite-avatica/blob/fcb1342a/server/src/test/java/org/apache/calcite/avatica/jdbc/JdbcMetaTest.java ---------------------------------------------------------------------- diff --git a/server/src/test/java/org/apache/calcite/avatica/jdbc/JdbcMetaTest.java b/server/src/test/java/org/apache/calcite/avatica/jdbc/JdbcMetaTest.java index d84fd29..f3ebc8a 100644 --- a/server/src/test/java/org/apache/calcite/avatica/jdbc/JdbcMetaTest.java +++ b/server/src/test/java/org/apache/calcite/avatica/jdbc/JdbcMetaTest.java @@ -31,10 +31,14 @@ import java.sql.ParameterMetaData; import java.sql.PreparedStatement; import java.sql.ResultSetMetaData; import java.sql.SQLException; +import java.util.Collections; +import java.util.Map; +import java.util.Properties; import java.util.UUID; import java.util.concurrent.atomic.AtomicInteger; import static org.hamcrest.CoreMatchers.is; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertThat; import static org.junit.Assert.fail; @@ -118,6 +122,56 @@ public class JdbcMetaTest { // Verify we called setMaxRows with the right value Mockito.verify(statement).setMaxRows(maxRows); } + + @Test public void testConcurrentConnectionOpening() throws Exception { + final Map<String, String> properties = Collections.emptyMap(); + final Connection conn = Mockito.mock(Connection.class); + // Override JdbcMeta to shim in a fake Connection object. Irrelevant for the test + JdbcMeta meta = new JdbcMeta("jdbc:url") { + @Override protected Connection createConnection(String url, Properties info) { + return conn; + } + }; + + ConnectionHandle ch1 = new ConnectionHandle("id1"); + meta.openConnection(ch1, properties); + + assertEquals(conn, meta.getConnectionCache().getIfPresent(ch1.id)); + try { + meta.openConnection(ch1, properties); + fail("Should not be allowed to open two connections with the same ID"); + } catch (RuntimeException e) { + // pass + } + // Cached object should not change + assertEquals(conn, meta.getConnectionCache().getIfPresent(ch1.id)); + } + + @Test public void testRacingConnectionOpens() throws Exception { + final Map<String, String> properties = Collections.emptyMap(); + final Connection conn1 = Mockito.mock(Connection.class); + final Connection conn2 = Mockito.mock(Connection.class); + final ConnectionHandle ch1 = new ConnectionHandle("id1"); + // Override JdbcMeta to shim in a fake Connection object. Irrelevant for the test + JdbcMeta meta = new JdbcMeta("jdbc:url") { + @Override protected Connection createConnection(String url, Properties info) { + // Hack to mimic the race condition of a connection being cached by another thread. + getConnectionCache().put(ch1.id, conn1); + // Return our "newly created" connectino + return conn2; + } + }; + + try { + meta.openConnection(ch1, properties); + fail("Should see an exception when the cache already contained our connection after" + + " creating it"); + } catch (RuntimeException e) { + // pass + } + // Our opened connection should get closed when this race condition happens + Mockito.verify(conn2).close(); + } } // End JdbcMetaTest.java
