This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 66aee08dc0e9b9e8e547bc8fcdc5fcd403baf0c4 Author: remm <[email protected]> AuthorDate: Wed Oct 28 16:07:54 2026 +0100 Claim the async pooled connection once, fixing a two-owner race The fair queue delivers the connection it produced to every caller of the future it hands out, and the cancellation of a ConnectionFuture used that same delivery to retrieve the connection and close it without establishing whether a borrower had taken delivery of it. A cancel() that landed while a borrower was receiving the connection from the queue, or was still setting it up, therefore reported a successful cancellation while the borrower was using that connection: the session went back to the pool, and to the next borrower, underneath its owner, or the owner received a handle that had already been closed. One physical connection could serve two users at once, silently. Make the connection claimed once, by whichever path gets there first. The compare-and-set on the "configured" flag now decides who may act on the connection: a get() caller, who then sets it up, publishes the result and owns the resulting Connection; or the cancellation, which arms the cleanup that sets the connection up the way a borrow does, so its return runs through the interceptors, and closes it without ever publishing a result. The losing path neither uses the connection nor claims a cancellation: cancel() reports false once a get() caller has claimed the connection, the way java.util.concurrent.Future reports nothing cancelled once the work is done, and a get() caller that lost the claim to a cancellation observes a CancellationException rather than a closed connection, on the untimed path, the timed path and the latch timeout alike. Cover it with deterministic tests that pin each interleaving with a gated queue future and a validator that parks a borrower inside the claim window: cancel while a borrower holds the connection, cancel just before the borrower claims it, cancel with no borrower at all, cancel before delivery, and two concurrent getters. --- .../apache/tomcat/jdbc/pool/ConnectionPool.java | 151 +++++-- .../jdbc/pool/TestAsyncBorrowCancelRace.java | 462 +++++++++++++++++++++ webapps/docs/changelog.xml | 22 + 3 files changed, 602 insertions(+), 33 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java index c20cac57ab..0bf15edc5f 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java @@ -1626,6 +1626,16 @@ public class ConnectionPool { */ protected class ConnectionFuture implements Future<Connection>, Runnable { Future<PooledConnection> pcFuture = null; + /** + * The claim on the pooled connection this future produces. The queue + * hands its connection to every caller of the queue future, so this + * single compare-and-set is what decides who may act on it: a + * {@link #get(long, TimeUnit)} caller, who then owns the connection + * that comes out of it, or {@link #cancel(boolean)}, whose cleanup + * task returns that connection to the pool. Whichever path loses the + * claim must neither use the connection nor report a cancellation of + * it, so one physical connection never ends up with two owners. + */ AtomicBoolean configured = new AtomicBoolean(false); CountDownLatch latch = new CountDownLatch(1); volatile Connection result = null; @@ -1663,20 +1673,31 @@ public class ConnectionPool { //deregister and no connection to clean up, so report nothing //cancelled without arming the deferred cleanup task, which a //java.util.concurrent.Future must not run for a completed task. - if (pc!=null || result!=null || pcFuture.isCancelled()) { + if (pc!=null || result!=null || cancelled.get() || pcFuture.isCancelled()) { return false; - } else if (cancelled.compareAndSet(false, true)) { - if (!pcFuture.cancel(mayInterruptIfRunning)) { - //The pooled connection was already handed over to the - //queue future (or is being handed over right now), so - //cancel by retrieving the connection and returning it - //to the pool - ConnectionPool.this.cancellator.execute(this); - } - return true; } - //already cancelled - return false; + //The queue may have produced its connection already, and a get() + //caller that received it is using it, so cancel only when this + //thread wins the claim on that connection. Losing the claim means + //the connection belongs to that caller, which is the completed + //case a java.util.concurrent.Future reports as not cancelled. + if (!configured.compareAndSet(false, true)) { + return false; + } + cancelled.set(true); + if (pcFuture.cancel(mayInterruptIfRunning)) { + //The waiter was still queued, so it is deregistered and no + //connection will be produced for this future. Release the + //callers that are waiting on the claim above, as nobody is + //left to publish a result for them. + latch.countDown(); + } else { + //The pooled connection was already handed over to the queue + //future (or is being handed over right now), so cancel by + //retrieving the connection and returning it to the pool + ConnectionPool.this.cancellator.execute(this); + } + return true; } /** @@ -1702,21 +1723,35 @@ public class ConnectionPool { return result; } if (configured.compareAndSet(false, true)) { - try { - pc = borrowConnection(System.currentTimeMillis(),pc, null, null); - if (pc != null) { - result = ConnectionPool.this.setupConnection(pc); + configure(pc); + } else { + //if we reach here, another thread claimed the connection: + //a get() caller that is setting it up, or the cancellation + //cleanup that returns it to the pool + if (!latch.await(timeout,unit)) { //this shouldn't block for long + if (cancelled.get()) { + //a cancellation won the claim, so its cleanup is + //what this wait was for: as for a cancelled + //java.util.concurrent.Future, its callers are told + //about the cancellation rather than about their + //own deadline expiring. A timeout observed before + //the cancellation wins the claim stays a timeout; + //that window is inherent and bounded, the next + //get() here observes the cancellation + throw new CancellationException(); } - } catch (SQLException x) { - cause = x; - } finally { - latch.countDown(); + throw new TimeoutException(); } - } else { - //if we reach here, another thread is configuring the actual connection - latch.await(timeout,unit); //this shouldn't block for long } if (result==null) { + if (cancelled.get()) { + //The cancellation cleanup won the claim on this + //connection and returned it to the pool, so it is not + //ours to hand out; the callers of a cancelled future + //are told about the cancellation instead of being + //handed a connection that is already back in the pool + throw new CancellationException(); + } throw new ExecutionException(cause); } return result; @@ -1725,6 +1760,27 @@ public class ConnectionPool { } } + /** + * Sets up the pooled connection that the claim on {@link #configured} + * gave to this caller and publishes what came out of it for the + * callers waiting on {@link #latch}. Only the winner of that claim may + * call this, which is what keeps the connection from being handed to + * one caller while the cancellation cleanup returns it to the pool. + * @param pooled the pooled connection the queue handed out + */ + private void configure(PooledConnection pooled) { + try { + PooledConnection pc = borrowConnection(System.currentTimeMillis(),pooled, null, null); + if (pc != null) { + result = ConnectionPool.this.setupConnection(pc); + } + } catch (SQLException x) { + cause = x; + } finally { + latch.countDown(); + } + } + /** * {@inheritDoc} */ @@ -1746,20 +1802,49 @@ public class ConnectionPool { */ @Override public void run() { + //cancel() armed this task because it won the claim on the + //connection this future produces, so this is the path that hands + //that connection back to the pool when no get() caller claimed it. + //It sets the connection up the way a borrow does, so that the + //return runs through the interceptors, but it never publishes a + //result: a get() caller waiting on the claim has to observe the + //cancellation rather than a connection that is already closed. try { - Connection con = get(); //complete this future - if (con != null) { - con.close(); //return to the pool + PooledConnection pooled = null; + try { + pooled = pcFuture.get(); //the connection this future produced + } catch (InterruptedException x) { + Thread.currentThread().interrupt(); + } catch (ExecutionException x) { + //the queue future produced no connection to return + } catch (CancellationException x) { + //the pool closure cancelled the queued wait between the + //registration of this cleanup task and its execution; this + //future never produced a connection, so there is nothing + //to close + } + if (pooled != null) { + try { + PooledConnection pc = + borrowConnection(System.currentTimeMillis(), pooled, null, null); + if (pc != null) { + Connection con = ConnectionPool.this.setupConnection(pc); + if (con != null) { + con.close(); //return to the pool + } + } + } catch (SQLException x) { + //the connection was released or could not be set up, + //so there is nothing left to return to the pool + cause = x; + } } - }catch (ExecutionException ex) { - //we can ignore this - }catch (CancellationException ex) { - //the pool closure cancelled the queued wait between the - //registration of this cleanup task and its execution; this - //future never produced a connection, so there is nothing - //to close }catch (Exception x) { log.error("Unable to cancel ConnectionFuture.",x); + } finally { + //whatever came out of it, the callers waiting on the claim are + //done waiting + latch.countDown(); } } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestAsyncBorrowCancelRace.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestAsyncBorrowCancelRace.java new file mode 100644 index 0000000000..88488df7d1 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestAsyncBorrowCancelRace.java @@ -0,0 +1,462 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tomcat.jdbc.pool; + +import java.sql.Connection; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.sql.Statement; +import java.util.concurrent.CancellationException; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; + +/** + * Expected behaviour of an asynchronous borrow that gets cancelled while + * the connection it produced is on its way to a borrower. + * + * <p>The queue future hands its {@code PooledConnection} to every caller of + * its {@code get()}, so the future that wraps it must make sure that exactly + * one path claims that connection: either a {@code get()} caller, who then + * owns the resulting {@code Connection}, or the cancellation cleanup, which + * returns it to the pool. A {@code cancel()} that lost that race has to + * report that nothing was cancelled, the way + * {@link java.util.concurrent.Future#cancel(boolean)} reports nothing + * cancelled for a task that has already completed.</p> + */ +class TestAsyncBorrowCancelRace extends JdbcPoolBaseTest { + + /** + * A queue future whose item has already been handed over, mirroring + * {@code FairBlockingQueue.ItemFuture}: its {@code get()} re-delivers the + * same pooled connection to every caller and its {@code cancel()} reports + * that nothing could be taken back. The first {@code get()} can be held + * inside the call, which is how these tests pin down an interleaving that + * would otherwise depend on thread scheduling. + */ + private static final class DeliveredFuture implements Future<PooledConnection> { + private final PooledConnection pooled; + private final CountDownLatch enteredGet; + private final CountDownLatch releaseGet; + private final AtomicBoolean gatesFirstGet; + + DeliveredFuture(PooledConnection pooled, CountDownLatch enteredGet, + CountDownLatch releaseGet, boolean gateFirstGet) { + this.pooled = pooled; + this.enteredGet = enteredGet; + this.releaseGet = releaseGet; + this.gatesFirstGet = new AtomicBoolean(gateFirstGet); + } + + @Override + public boolean cancel(boolean mayInterruptIfRunning) { + // an item that was already handed over cannot be taken back + return false; + } + + @Override + public PooledConnection get() throws InterruptedException, ExecutionException { + return doGet(); + } + + @Override + public PooledConnection get(long timeout, TimeUnit unit) + throws InterruptedException, ExecutionException, TimeoutException { + return doGet(); + } + + private PooledConnection doGet() throws InterruptedException { + if (gatesFirstGet.compareAndSet(true, false)) { + enteredGet.countDown(); + releaseGet.await(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS); + } + return pooled; + } + + @Override + public boolean isCancelled() { + return false; + } + + @Override + public boolean isDone() { + return true; + } + } + + /** + * A validator the pool calls while a borrower sets an idle connection up, + * which is inside the window in which that borrower owns the connection + * but has not handed it out yet. The first call marks the physical + * session and parks the borrower until the test releases it. It is + * configured through the pool properties, so it must stay a public static + * class with a no-arg constructor. + */ + public static class ParkingValidator implements Validator { + volatile CountDownLatch insideSetup; + volatile CountDownLatch resumeSetup; + private final AtomicInteger calls = new AtomicInteger(); + + @Override + public boolean validate(Connection connection, int validateAction) { + CountDownLatch inside = insideSetup; + CountDownLatch resume = resumeSetup; + if (inside != null && resume != null && calls.incrementAndGet() == 1) { + try (Statement st = connection.createStatement()) { + st.execute("SET @BORROW_TEST_MARKER = 42"); + } catch (SQLException x) { + throw new IllegalStateException(x); + } + inside.countDown(); + try { + resume.await(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS); + } catch (InterruptedException x) { + Thread.currentThread().interrupt(); + } + } + return true; + } + } + + /* + * The H2 session variable a ParkingValidator leaves behind, read through + * a borrowed connection: it says which physical session that connection + * maps to. + */ + private static Integer borrowMarker(Connection c) throws SQLException { + try (Statement st = c.createStatement(); + ResultSet rs = st.executeQuery("SELECT @BORROW_TEST_MARKER")) { + rs.next(); + return rs.getObject(1) == null ? null : Integer.valueOf(rs.getInt(1)); + } + } + + @Test + void cancellingWhileTheBorrowerHoldsTheConnectionReportsNothingCancelled() throws Exception { + ParkingValidator validator = new ParkingValidator(); + DataSource ds = newDataSource(); + ds.getPoolProperties().setFairQueue(true); + ds.getPoolProperties().setInitialSize(0); + ds.getPoolProperties().setMinIdle(0); + ds.getPoolProperties().setMaxActive(3); + ds.getPoolProperties().setMaxIdle(3); + ds.getPoolProperties().setTestOnBorrow(true); + ds.getPoolProperties().setValidationInterval(0); + ds.getPoolProperties().setValidator(validator); + ds.createPool(); + ConnectionPool pool = ds.getPool(); + long returnedBefore = pool.getReturnedCount(); + + Connection holder = ds.getConnection(); + Connection other = ds.getConnection(); + Connection third = ds.getConnection(); // the pool is at maxActive now + + // this borrow can only be served by a returned connection + Future<Connection> future = ds.getConnectionAsync(); + Assertions.assertFalse(future.isDone(), + "the poll must still be waiting for a connection"); + + CountDownLatch insideSetup = new CountDownLatch(1); + CountDownLatch resumeSetup = new CountDownLatch(1); + validator.insideSetup = insideSetup; + validator.resumeSetup = resumeSetup; + + AtomicReference<Connection> borrowed = new AtomicReference<>(); + AtomicReference<Throwable> failure = new AtomicReference<>(); + Thread caller = new Thread(() -> { + try { + borrowed.set(future.get(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS)); + } catch (Throwable e) { + failure.set(e); + } + }, "async-borrower"); + caller.setDaemon(true); + caller.start(); + + holder.close(); // hands this borrower's connection to the queued poll + Assertions.assertTrue(insideSetup.await(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS), + "the borrower must have taken a connection out of the queue"); + Assertions.assertTrue(caller.isAlive(), + "the borrower must still be setting that connection up"); + + // a watchdog gives up on the borrow while its caller is using it + Assertions.assertFalse(future.cancel(true), + "the caller that claimed the connection must not be cancelled " + + "out from under it, and the connection must not go " + + "back to the pool underneath it"); + resumeSetup.countDown(); + caller.join(AWAIT_TIMEOUT_MS); + Assertions.assertFalse(caller.isAlive(), "the borrower must have finished"); + + Assertions.assertNull(failure.get(), + "the borrower must still be served its connection, got " + failure.get()); + Connection connection = borrowed.get(); + Assertions.assertNotNull(connection, "the borrower must have received a connection"); + Assertions.assertEquals(Integer.valueOf(42), borrowMarker(connection), + "the borrower must hold the session it took out of the queue"); + + // that connection must not have been returned underneath its owner + Assertions.assertEquals(1, pool.getReturnedCount() - returnedBefore, + "only the borrower's connection may go back to the pool"); + Assertions.assertEquals(0, ds.getIdle(), + "the connection a caller holds must not be up for grabs again"); + Assertions.assertEquals(3, ds.getActive()); + + // the caller keeps a usable connection, and it goes back exactly once + try (ResultSet rs = connection.createStatement().executeQuery("SELECT 1")) { + Assertions.assertTrue(rs.next(), "the borrowed connection must still work"); + } + connection.close(); + Assertions.assertEquals(2, pool.getReturnedCount() - returnedBefore, + "the connection must be returned once, by the one who owns it"); + Assertions.assertEquals(1, ds.getIdle()); + + other.close(); + third.close(); + ds.close(); + awaitSessions(0); + } + + @Test + void cancellingBeforeTheBorrowerClaimsTheConnectionReturnsItToThePool() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setFairQueue(true); + ds.getPoolProperties().setInitialSize(0); + ds.getPoolProperties().setMinIdle(0); + ds.getPoolProperties().setMaxActive(2); + ds.getPoolProperties().setMaxIdle(2); + ds.createPool(); + ConnectionPool pool = ds.getPool(); + long returnedBefore = pool.getReturnedCount(); + + // a pooled connection the queue has already handed to the poll + PooledConnection pooled = pool.create(true); + CountDownLatch insideQueueGet = new CountDownLatch(1); + CountDownLatch releaseQueueGet = new CountDownLatch(1); + Future<PooledConnection> queueFuture = + new DeliveredFuture(pooled, insideQueueGet, releaseQueueGet, true); + Future<Connection> future = pool.new ConnectionFuture(queueFuture); + + AtomicReference<Connection> borrowed = new AtomicReference<>(); + AtomicReference<Throwable> failure = new AtomicReference<>(); + Thread caller = new Thread(() -> { + try { + borrowed.set(future.get(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS)); + } catch (Throwable e) { + failure.set(e); + } + }, "async-borrower"); + caller.setDaemon(true); + caller.start(); + Assertions.assertTrue(insideQueueGet.await(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS), + "the borrower must hold the connection the queue produced"); + + // the watchdog claims that connection, so it is the one that reclaims it + Assertions.assertTrue(future.cancel(true), + "a cancellation that claims the produced connection must report it"); + releaseQueueGet.countDown(); + caller.join(AWAIT_TIMEOUT_MS); + + Assertions.assertNull(borrowed.get(), "a cancelled borrow must not hand out the " + + "connection the cleanup reclaimed, it handed out " + borrowed.get()); + Assertions.assertTrue(failure.get() instanceof CancellationException, + "a cancelled borrow must fail its caller with a cancellation, got " + + failure.get()); + Assertions.assertTrue(future.isCancelled()); + Assertions.assertTrue(future.isDone()); + + // the connection the cleanup claimed is back in the pool, exactly once + awaitUntil(() -> pool.getReturnedCount() - returnedBefore == 1); + sleep(200); // a second return by the same cleanup would show up now + Assertions.assertEquals(1, pool.getReturnedCount() - returnedBefore, + "the cancellation cleanup must return the connection exactly once"); + Assertions.assertEquals(1, ds.getIdle(), + "the reclaimed connection must be idle again"); + Assertions.assertEquals(0, ds.getActive()); + + // and it is still there for the next borrower + try (Connection c = ds.getConnection()) { + Assertions.assertEquals(1, ds.getActive()); + } + ds.close(); + } + + @Test + void cancellingADeliveredPollWithNoBorrowerReclaimsTheConnection() throws Exception { + ParkingValidator validator = new ParkingValidator(); + DataSource ds = newDataSource(); + ds.getPoolProperties().setFairQueue(true); + ds.getPoolProperties().setInitialSize(0); + ds.getPoolProperties().setMinIdle(0); + ds.getPoolProperties().setMaxActive(2); + ds.getPoolProperties().setMaxIdle(2); + ds.getPoolProperties().setTestOnBorrow(true); + ds.getPoolProperties().setValidationInterval(0); + ds.getPoolProperties().setValidator(validator); + ds.createPool(); + ConnectionPool pool = ds.getPool(); + long returnedBefore = pool.getReturnedCount(); + + // the queue produced its connection but nobody calls get(): this is + // the documented watchdog case, cancelling must reclaim the connection + PooledConnection pooled = pool.create(true); + CountDownLatch unused = new CountDownLatch(0); + Future<PooledConnection> queueFuture = + new DeliveredFuture(pooled, unused, unused, false); + Future<Connection> future = pool.new ConnectionFuture(queueFuture); + + // the validator must be armed before the cancellation, as the + // cancellation starts the cleanup right away and that cleanup has to + // park inside the borrow it does with the produced connection + CountDownLatch insideCleanup = new CountDownLatch(1); + CountDownLatch resumeCleanup = new CountDownLatch(1); + validator.insideSetup = insideCleanup; + validator.resumeSetup = resumeCleanup; + + Assertions.assertTrue(future.cancel(true), + "a delivered poll nobody claimed must be cancellable"); + Assertions.assertTrue(insideCleanup.await(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS), + "the cleanup must have taken the produced connection into its" + + " borrow before the timed getter runs"); + + // a timed get() that loses the claim to that running cancellation is a + // caller of a cancelled future: it must be told about the cancellation + // rather than about its own deadline expiring + Assertions.assertThrows(CancellationException.class, + () -> future.get(100, TimeUnit.MILLISECONDS), + "a timed get() that loses the claim to a won cancellation must" + + " report the cancellation, not a timeout"); + + resumeCleanup.countDown(); + awaitUntil(() -> pool.getReturnedCount() - returnedBefore == 1); + sleep(200); // a second return by the same cleanup would show up now + Assertions.assertEquals(1, pool.getReturnedCount() - returnedBefore, + "the cleanup must return the connection exactly once"); + Assertions.assertEquals(1, ds.getIdle()); + Assertions.assertEquals(0, ds.getActive()); + Assertions.assertTrue(future.isCancelled()); + + // a caller that shows up late is told about the cancellation instead + // of being handed the connection that already went back to the pool + Assertions.assertThrows(CancellationException.class, + () -> future.get(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS), + "a completed cancellation must keep being reported"); + + // and the connection is still there for the next borrower + try (Connection c = ds.getConnection()) { + Assertions.assertEquals(1, ds.getActive()); + } + ds.close(); + } + + @Test + void cancellingBeforeDeliveryDeregistersThePollAndFailsItsGetters() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setFairQueue(true); + ds.getPoolProperties().setInitialSize(0); + ds.getPoolProperties().setMinIdle(0); + ds.getPoolProperties().setMaxActive(1); + ds.getPoolProperties().setMaxIdle(1); + ds.createPool(); + + Connection holder = ds.getConnection(); + setSessionMarker(holder, 7); + Future<Connection> future = ds.getConnectionAsync(); + + // the poll is still queued, so cancelling it takes nothing back and + // hands nothing to anybody + Assertions.assertTrue(future.cancel(true), "a queued poll must be cancellable"); + Assertions.assertTrue(future.isCancelled()); + Assertions.assertTrue(future.isDone()); + Assertions.assertThrows(CancellationException.class, + () -> future.get(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS), + "a cancelled poll must fail its caller with a cancellation"); + Assertions.assertFalse(future.cancel(true), + "a second cancel must report nothing cancelled"); + + // the cancelled poll must not consume the connection it waited for + holder.close(); + Assertions.assertEquals(1, ds.getIdle()); + try (Connection c = ds.getConnection()) { + Assertions.assertEquals(Integer.valueOf(7), sessionMarker(c), + "the connection must go to the next borrower, not to the " + + "cancelled poll"); + } + ds.close(); + } + + @Test + void concurrentGettersShareOneConnectionAndItIsReturnedOnce() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setFairQueue(true); + ds.getPoolProperties().setInitialSize(0); + ds.getPoolProperties().setMinIdle(0); + ds.getPoolProperties().setMaxActive(2); + ds.getPoolProperties().setMaxIdle(2); + ds.createPool(); + ConnectionPool pool = ds.getPool(); + long returnedBefore = pool.getReturnedCount(); + + PooledConnection pooled = pool.create(true); + CountDownLatch unused = new CountDownLatch(0); + Future<PooledConnection> queueFuture = + new DeliveredFuture(pooled, unused, unused, false); + final Future<Connection> future = pool.new ConnectionFuture(queueFuture); + + AtomicInteger served = new AtomicInteger(); + AtomicReference<Connection> handle = new AtomicReference<>(); + AtomicReference<Throwable> failure = new AtomicReference<>(); + Runnable getter = () -> { + try { + Connection c = future.get(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS); + handle.compareAndSet(null, c); + served.incrementAndGet(); + } catch (Throwable e) { + failure.compareAndSet(null, e); + } + }; + Thread a = new Thread(getter, "getter-a"); + Thread b = new Thread(getter, "getter-b"); + a.setDaemon(true); + b.setDaemon(true); + a.start(); + b.start(); + a.join(AWAIT_TIMEOUT_MS); + b.join(AWAIT_TIMEOUT_MS); + + Assertions.assertNull(failure.get(), + "both getters must be served, got " + failure.get()); + Assertions.assertEquals(2, served.get()); + Assertions.assertEquals(1, ds.getActive(), + "one connection must be borrowed, not two"); + handle.get().close(); + Assertions.assertEquals(1, pool.getReturnedCount() - returnedBefore, + "the shared connection must be returned once"); + Assertions.assertEquals(1, ds.getIdle()); + ds.close(); + } +} diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 75d7f7390f..3c9500a22a 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -1259,6 +1259,28 @@ a borrow that had registered its waiter just before the closure ran gets a future that reports that cancellation instead. (remm) </fix> + <fix> + Correct a defect in the asynchronous borrow of the JDBC connection pool + that handed one physical connection to two users at once, silently + sharing a session between them. The queue delivers the connection it + produced to every caller of its <code>get()</code>, and the cancellation + of the future that <code>getConnectionAsync()</code> had returned used + that same delivery to retrieve the connection and close it, without + establishing whether a borrower had taken delivery of it. A + <code>cancel()</code> that landed while a borrower was receiving the + connection, or was still setting it up, therefore reported a successful + cancellation while the borrower was using that connection: it went back + to the pool and to the next borrower underneath its owner, or the owner + was handed a connection that had already been closed. The connection the + queue produces is now claimed once, by whichever of the borrower and the + cancellation gets there first. A borrower that won the claim keeps its + connection and <code>cancel()</code> reports that nothing was cancelled, + the way <code>java.util.concurrent.Future</code> reports nothing + cancelled once the work is done; a cancellation that won it returns the + connection to the pool exactly once and the future reports a + <code>CancellationException</code> to its callers instead of a closed + connection. (remm) + </fix> </changelog> </subsection> <subsection name="Other"> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
