Skip to content

Commit

Permalink
fix: replace syncronization in Connection.close with compareAndSet
Browse files Browse the repository at this point in the history
Statement can be synchronized during query execution, and we don't want
.close() to wait for the statement completion.
  • Loading branch information
vlsi committed Jul 27, 2022
1 parent 4673fd2 commit 736f959
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 10 deletions.
14 changes: 7 additions & 7 deletions pgjdbc/src/main/java/org/postgresql/jdbc/PgStatement.java
Original file line number Diff line number Diff line change
Expand Up @@ -520,7 +520,10 @@ public void setCursorName(String name) throws SQLException {
// No-op.
}

private volatile boolean isClosed = false;
private volatile int isClosed = 0;
private static final AtomicIntegerFieldUpdater<PgStatement> IS_CLOSED_UPDATER =
AtomicIntegerFieldUpdater.newUpdater(
PgStatement.class, "isClosed");

@Override
public int getUpdateCount() throws SQLException {
Expand Down Expand Up @@ -673,11 +676,8 @@ public void clearWarnings() throws SQLException {
*/
public final void close() throws SQLException {
// closing an already closed Statement is a no-op.
synchronized (this) {
if (isClosed) {
return;
}
isClosed = true;
if (!IS_CLOSED_UPDATER.compareAndSet(this, 0, 1)) {
return;
}

cancel();
Expand Down Expand Up @@ -1138,7 +1138,7 @@ public long executeLargeUpdate(String sql, String @Nullable [] columnNames) thro
}

public boolean isClosed() throws SQLException {
return isClosed;
return isClosed == 1;

This comment has been minimized.

Copy link
@KevinJCross

KevinJCross Aug 10, 2022

@vlsi : I believe this may not be consistently correct. Since synchronised has been removed from the previous updated the java memory model does not give you any guantees that this is up to date in this thread.
It would be more sensible to use the get method on the IS_CLOSED_UPDATER i.e. IS_CLOSED_UPDATER.get().
This should at least give some guarantee that the result is fresh and not cached for this thread.

This comment has been minimized.

Copy link
@vlsi

vlsi Aug 10, 2022

Author Member

isClosed is volatile. Does your statement still hold with that?

}

public void setPoolable(boolean poolable) throws SQLException {
Expand Down
19 changes: 16 additions & 3 deletions pgjdbc/src/test/java/org/postgresql/test/jdbc2/StatementTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@
import java.sql.SQLException;
import java.sql.SQLWarning;
import java.sql.Statement;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Properties;
import java.util.Random;
Expand Down Expand Up @@ -861,7 +863,7 @@ public void testMultipleCancels() throws Exception {
public void testCancelQueryWithBrokenNetwork() throws SQLException, IOException, InterruptedException {
// check that stmt.cancel() doesn't hang forever if the network is broken

ExecutorService executor = Executors.newSingleThreadExecutor();
ExecutorService executor = Executors.newCachedThreadPool();

try (StrangeProxyServer proxyServer = new StrangeProxyServer(TestUtil.getServer(), TestUtil.getPort())) {
Properties props = new Properties();
Expand All @@ -875,6 +877,9 @@ public void testCancelQueryWithBrokenNetwork() throws SQLException, IOException,
proxyServer.stopForwardingAllClients();

stmt.cancel();
// Note: network is still inaccessible, so the statement execution is still in progress.
// So we abort the connection to allow implicit conn.close()
conn.abort(executor);
}
}

Expand Down Expand Up @@ -940,12 +945,13 @@ public Void call() throws Exception {
}

@Test(timeout = 10000)
public void testConcurrentIsValid() throws InterruptedException {
public void testConcurrentIsValid() throws Throwable {
ExecutorService executor = Executors.newCachedThreadPool();
try {
List<Future<?>> results = new ArrayList<>();
Random rnd = new Random();
for (int i = 0; i < 10; i++) {
executor.submit(() -> {
Future<?> future = executor.submit(() -> {
try {
for (int j = 0; j < 50; j++) {
con.isValid(1);
Expand All @@ -969,7 +975,14 @@ public void testConcurrentIsValid() throws InterruptedException {
throw new RuntimeException(e);
}
});
results.add(future);
}
for (Future<?> result : results) {
// Propagate exception if any
result.get();
}
} catch (ExecutionException e) {
throw e.getCause();
} finally {
executor.shutdown();
executor.awaitTermination(10, TimeUnit.SECONDS);
Expand Down

0 comments on commit 736f959

Please sign in to comment.