From 0be98d798924b8fedfa94ab0124f1f5a8b266fef Mon Sep 17 00:00:00 2001 From: Rob Bygrave Date: Wed, 3 Feb 2021 14:51:42 +1300 Subject: [PATCH] Fix to only cancel query once (#2152) Change DefaultOrmQuery.cancel() to call underlying jdbc cancel once - Refactor tidy already cancelled check (pre query execution) - Remove unnecessary extra transaction.end() call on future query execution (as already handled by CallableQueryList etc) --- .../io/ebeaninternal/server/query/CQuery.java | 24 +++++-------------- .../server/query/CQueryEngine.java | 6 ----- .../server/querydefn/DefaultOrmQuery.java | 14 ++--------- .../tests/query/TestQueryFindFutureList.java | 20 +++++++++------- 4 files changed, 19 insertions(+), 45 deletions(-) diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/query/CQuery.java b/ebean-core/src/main/java/io/ebeaninternal/server/query/CQuery.java index f92960419..861ccc337 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/query/CQuery.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/query/CQuery.java @@ -296,10 +296,10 @@ public class CQuery implements DbReadContext, CancelableQuery, SpiProfileTran this.cancelled = true; if (pstmt != null) { try { + logger.debug("Cancelling query"); pstmt.cancel(); } catch (SQLException e) { - String msg = "Error cancelling query"; - throw new PersistenceException(msg, e); + throw new PersistenceException("Error cancelling query", e); } } } finally { @@ -322,7 +322,6 @@ public class CQuery implements DbReadContext, CancelableQuery, SpiProfileTran } private boolean prepareBindExecuteQueryWithOption(boolean forwardOnlyHint) throws SQLException { - ResultSet resultSet = prepareResultSet(forwardOnlyHint); if (resultSet == null) { return false; @@ -334,19 +333,12 @@ public class CQuery implements DbReadContext, CancelableQuery, SpiProfileTran ResultSet prepareResultSet(boolean forwardOnlyHint) throws SQLException { lock.lock(); try { - if (cancelled || query.isCancelled()) { - // cancelled before we started - cancelled = true; - return null; + if (cancelled) { + throw new SQLException("Query cancelled"); } - startNano = System.nanoTime(); - - // prepare SpiTransaction t = request.getTransaction(); profileOffset = t.profileOffset(); - Connection conn = t.getInternalConnection(); - if (query.isRawSql()) { ResultSet suppliedResultSet = query.getRawSql().getResultSet(); if (suppliedResultSet != null) { @@ -356,6 +348,7 @@ public class CQuery implements DbReadContext, CancelableQuery, SpiProfileTran } } + Connection conn = t.getInternalConnection(); if (forwardOnlyHint) { // Use forward only hints for large resultSet processing (Issue 56, MySql specific) pstmt = conn.prepareStatement(sql, ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_READ_ONLY); @@ -363,18 +356,13 @@ public class CQuery implements DbReadContext, CancelableQuery, SpiProfileTran } else { pstmt = conn.prepareStatement(sql); } - if (query.getTimeout() > 0) { pstmt.setQueryTimeout(query.getTimeout()); } if (query.getBufferFetchSizeHint() > 0) { pstmt.setFetchSize(query.getBufferFetchSizeHint()); } - - DataBind dataBind = queryPlan.bindEncryptedProperties(pstmt, conn); - bindLog = predicates.bind(dataBind); - - // executeQuery + bindLog = predicates.bind(queryPlan.bindEncryptedProperties(pstmt, conn)); return pstmt.executeQuery(); } finally { lock.unlock(); diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryEngine.java b/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryEngine.java index f462050fb..945b343ca 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryEngine.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryEngine.java @@ -401,12 +401,6 @@ public class CQueryEngine { if (cquery != null) { cquery.close(); } - if (request.getQuery().isFutureFetch()) { - // end the transaction for futureFindIds - // as it had it's own transaction - logger.debug("Future fetch completed!"); - request.getTransaction().end(); - } } } diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/querydefn/DefaultOrmQuery.java b/ebean-core/src/main/java/io/ebeaninternal/server/querydefn/DefaultOrmQuery.java index 39d7b350d..2f73bceb7 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/querydefn/DefaultOrmQuery.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/querydefn/DefaultOrmQuery.java @@ -751,7 +751,6 @@ public class DefaultOrmQuery implements SpiQuery { @Override public NaturalKeyQueryData naturalKey() { - if (whereExpressions == null) { return null; } @@ -767,7 +766,6 @@ public class DefaultOrmQuery implements SpiQuery { return null; } } - return data; } @@ -815,7 +813,6 @@ public class DefaultOrmQuery implements SpiQuery { copy.m2mIncludeJoin = m2mIncludeJoin; copy.profilingListener = profilingListener; copy.profileLocation = profileLocation; - copy.baseTable = baseTable; copy.rootTableAlias = rootTableAlias; copy.distinct = distinct; @@ -1100,7 +1097,6 @@ public class DefaultOrmQuery implements SpiQuery { @Override public ObjectGraphNode setOrigin(CallOrigin callOrigin) { - // create a 'origin' which links this query to the profiling information ObjectGraphOrigin o = new ObjectGraphOrigin(calculateOriginQueryHash(), callOrigin, beanType.getName()); parentNode = new ObjectGraphNode(o, null); @@ -1241,7 +1237,6 @@ public class DefaultOrmQuery implements SpiQuery { */ @Override public CQueryPlanKey prepare(SpiOrmQueryRequest request) { - prepareExpressions(request); prepareForPaging(); queryPlanKey = createQueryPlanKey(); @@ -1252,7 +1247,6 @@ public class DefaultOrmQuery implements SpiQuery { * Prepare the expressions (compile sub-queries etc). */ private void prepareExpressions(BeanQueryRequest request) { - if (whereExpressions != null) { whereExpressions.prepareExpression(request); } @@ -1267,7 +1261,6 @@ public class DefaultOrmQuery implements SpiQuery { * case, this is not a distinct query */ private void prepareForPaging() { - // add the rawSql statement - if any if (orderByIsEmpty()) { if (rawSql != null && rawSql.getSql() != null) { @@ -1678,7 +1671,6 @@ public class DefaultOrmQuery implements SpiQuery { return this; } } - if (bindParams == null) { bindParams = new BindParams(); } @@ -1972,7 +1964,6 @@ public class DefaultOrmQuery implements SpiQuery { if (namedParams == null) { namedParams = new HashMap<>(); } - return namedParams.computeIfAbsent(name, ONamedParam::new); } @@ -2066,8 +2057,8 @@ public class DefaultOrmQuery implements SpiQuery { public void cancel() { lock.lock(); try { - cancelled = true; - if (cancelableQuery != null) { + if (!cancelled && cancelableQuery != null) { + cancelled = true; cancelableQuery.cancel(); } } finally { @@ -2095,7 +2086,6 @@ public class DefaultOrmQuery implements SpiQuery { */ @Override public Set validate(BeanType desc) { - SpiExpressionValidation validation = new SpiExpressionValidation(desc); if (whereExpressions != null) { whereExpressions.validate(validation); diff --git a/ebean-core/src/test/java/org/tests/query/TestQueryFindFutureList.java b/ebean-core/src/test/java/org/tests/query/TestQueryFindFutureList.java index 6fb623e06..074603e41 100644 --- a/ebean-core/src/test/java/org/tests/query/TestQueryFindFutureList.java +++ b/ebean-core/src/test/java/org/tests/query/TestQueryFindFutureList.java @@ -1,7 +1,7 @@ package org.tests.query; import io.ebean.BaseTestCase; -import io.ebean.Ebean; +import io.ebean.DB; import io.ebean.FutureList; import io.ebean.Transaction; import org.tests.model.basic.Order; @@ -22,17 +22,19 @@ public class TestQueryFindFutureList extends BaseTestCase { ResetBasicData.reset(); // warm the connection pool - Transaction t0 = Ebean.getServer(null).createTransaction(); - Transaction t1 = Ebean.getServer(null).createTransaction(); - Transaction t2 = Ebean.getServer(null).createTransaction(); + Transaction t0 = DB.createTransaction(); + Transaction t1 = DB.createTransaction(); + Transaction t2 = DB.createTransaction(); t0.end(); t1.end(); t2.end(); - FutureList futureList = Ebean.find(Order.class).findFutureList(); + FutureList futureList = DB.find(Order.class).findFutureList(); Thread.sleep(10); futureList.cancel(true); + // calling again is ignored + futureList.cancel(true); // don't shutdown immediately Thread.sleep(50); @@ -43,12 +45,12 @@ public class TestQueryFindFutureList extends BaseTestCase { ResetBasicData.reset(); - FutureList futureList = Ebean.find(Order.class).findFutureList(); + FutureList futureList = DB.find(Order.class).findFutureList(); // wait for it to complete List orders = futureList.getUnchecked(); - assertEquals(Ebean.find(Order.class).findCount(), orders.size()); + assertEquals(DB.find(Order.class).findCount(), orders.size()); } @Test @@ -56,12 +58,12 @@ public class TestQueryFindFutureList extends BaseTestCase { ResetBasicData.reset(); - FutureList futureList = Ebean.find(Order.class).findFutureList(); + FutureList futureList = DB.find(Order.class).findFutureList(); // wait for it to complete List orders = futureList.getUnchecked(1, TimeUnit.SECONDS); - assertEquals(Ebean.find(Order.class).findCount(), orders.size()); + assertEquals(DB.find(Order.class).findCount(), orders.size()); } }