From 1deece0b5cac58511cbd7e34851f49e4f477d7e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yoann=20Rodi=C3=A8re?= Date: Wed, 13 Jan 2021 13:37:32 +0100 Subject: [PATCH 1/7] Test the transaction lifecycle (session flushes, session closing, ...) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Yoann Rodière --- .../AbstractTransactionLifecycleTest.java | 191 ++++++++++++++++++ .../GetTransactionLifecycleTest.java | 59 ++++++ .../orm/transaction/SimpleEntity.java | 36 ++++ .../TransactionAnnotationLifecycleTest.java | 65 ++++++ .../UserTransactionLifecycleTest.java | 65 ++++++ 5 files changed, 416 insertions(+) create mode 100644 extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/transaction/AbstractTransactionLifecycleTest.java create mode 100644 extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/transaction/GetTransactionLifecycleTest.java create mode 100644 extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/transaction/SimpleEntity.java create mode 100644 extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/transaction/TransactionAnnotationLifecycleTest.java create mode 100644 extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/transaction/UserTransactionLifecycleTest.java diff --git a/extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/transaction/AbstractTransactionLifecycleTest.java b/extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/transaction/AbstractTransactionLifecycleTest.java new file mode 100644 index 0000000000000..c7ebb5fb4a1ab --- /dev/null +++ b/extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/transaction/AbstractTransactionLifecycleTest.java @@ -0,0 +1,191 @@ +package io.quarkus.hibernate.orm.transaction; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Objects; +import java.util.function.Function; +import java.util.logging.Level; +import java.util.logging.LogRecord; + +import javax.persistence.EntityManager; + +import org.hibernate.BaseSessionEventListener; +import org.hibernate.Session; +import org.hibernate.engine.spi.SessionImplementor; +import org.hibernate.engine.spi.SharedSessionContractImplementor; +import org.jboss.shrinkwrap.api.ShrinkWrap; +import org.jboss.shrinkwrap.api.spec.JavaArchive; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +import io.quarkus.test.QuarkusUnitTest; + +/** + * Check transaction lifecycle, including session flushes and the closing of the session. + */ +public abstract class AbstractTransactionLifecycleTest { + + private static final String INITIAL_NAME = "Initial name"; + private static final String UPDATED_NAME = "Updated name"; + + @RegisterExtension + static QuarkusUnitTest runner = new QuarkusUnitTest() + .setArchiveProducer(() -> ShrinkWrap.create(JavaArchive.class) + .addClass(SimpleEntity.class) + .addAsResource("application.properties")) + // Expect no warnings (in particular from Agroal) + .setLogRecordPredicate(record -> record.getLevel().intValue() >= Level.WARNING.intValue() + // Ignore this particular warning when building with Java 8: it's not relevant to this test. + && !record.getMessage().contains("Using Java versions older than 11 to build Quarkus applications")) + .assertLogRecords(records -> assertThat(records) + .extracting(LogRecord::getMessage) // This is just to get meaningful error messages, as LogRecord doesn't have a toString() + .isEmpty()); + + @Test + public void testLifecycle() { + long id = 1L; + TestCRUD crud = crud(); + + ValueAndExecutionMetadata created = crud.create(id, INITIAL_NAME); + checkPostConditions(created, + LifecycleOperation.FLUSH, LifecycleOperation.STATEMENT, // update + expectDoubleFlush() ? LifecycleOperation.FLUSH : null, + LifecycleOperation.TRANSACTION_COMPLETION); + + ValueAndExecutionMetadata retrieved = crud.retrieve(id); + checkPostConditions(retrieved, + LifecycleOperation.STATEMENT, // select + LifecycleOperation.FLUSH, + expectDoubleFlush() ? LifecycleOperation.FLUSH : null, + LifecycleOperation.TRANSACTION_COMPLETION); + assertThat(retrieved.value).isEqualTo(INITIAL_NAME); + + ValueAndExecutionMetadata updated = crud.update(id, UPDATED_NAME); + checkPostConditions(updated, + LifecycleOperation.STATEMENT, // select + LifecycleOperation.FLUSH, LifecycleOperation.STATEMENT, // update + expectDoubleFlush() ? LifecycleOperation.FLUSH : null, + LifecycleOperation.TRANSACTION_COMPLETION); + + retrieved = crud.retrieve(id); + checkPostConditions(retrieved, + LifecycleOperation.STATEMENT, // select + LifecycleOperation.FLUSH, + expectDoubleFlush() ? LifecycleOperation.FLUSH : null, + LifecycleOperation.TRANSACTION_COMPLETION); + assertThat(retrieved.value).isEqualTo(UPDATED_NAME); + + ValueAndExecutionMetadata deleted = crud.delete(id); + checkPostConditions(deleted, + LifecycleOperation.STATEMENT, // select + LifecycleOperation.FLUSH, LifecycleOperation.STATEMENT, // delete + // No double flush here, since there's nothing in the session after the first flush. + LifecycleOperation.TRANSACTION_COMPLETION); + + retrieved = crud.retrieve(id); + checkPostConditions(retrieved, + LifecycleOperation.STATEMENT, // select + LifecycleOperation.TRANSACTION_COMPLETION); + assertThat(retrieved.value).isNull(); + } + + protected abstract TestCRUD crud(); + + protected abstract boolean expectDoubleFlush(); + + private void checkPostConditions(ValueAndExecutionMetadata result, LifecycleOperation... expectedOperationsArray) { + List expectedOperations = new ArrayList<>(); + Collections.addAll(expectedOperations, expectedOperationsArray); + expectedOperations.removeIf(Objects::isNull); + // No extra statements or flushes + assertThat(result.listener.operations) + .containsExactlyElementsOf(expectedOperations); + // Session was closed automatically + assertThat(result.sessionImplementor).returns(true, SharedSessionContractImplementor::isClosed); + } + + public abstract static class TestCRUD { + public ValueAndExecutionMetadata create(long id, String name) { + return inTransaction(entityManager -> { + SimpleEntity entity = new SimpleEntity(name); + entity.setId(id); + entityManager.persist(entity); + return null; + }); + } + + public ValueAndExecutionMetadata retrieve(long id) { + return inTransaction(entityManager -> { + SimpleEntity entity = entityManager.find(SimpleEntity.class, id); + return entity == null ? null : entity.getName(); + }); + } + + public ValueAndExecutionMetadata update(long id, String name) { + return inTransaction(entityManager -> { + SimpleEntity entity = entityManager.find(SimpleEntity.class, id); + entity.setName(name); + return null; + }); + } + + public ValueAndExecutionMetadata delete(long id) { + return inTransaction(entityManager -> { + SimpleEntity entity = entityManager.find(SimpleEntity.class, id); + entityManager.remove(entity); + return null; + }); + } + + public abstract ValueAndExecutionMetadata inTransaction(Function action); + } + + protected static class ValueAndExecutionMetadata { + + public static ValueAndExecutionMetadata run(EntityManager entityManager, Function action) { + LifecycleListener listener = new LifecycleListener(); + entityManager.unwrap(Session.class).addEventListeners(listener); + T result = action.apply(entityManager); + return new ValueAndExecutionMetadata<>(result, entityManager, listener); + } + + final T value; + final SessionImplementor sessionImplementor; + final LifecycleListener listener; + + private ValueAndExecutionMetadata(T value, EntityManager entityManager, LifecycleListener listener) { + this.value = value; + // Make sure we don't return a wrapper, but the actual implementation. + this.sessionImplementor = entityManager.unwrap(SessionImplementor.class); + this.listener = listener; + } + } + + private static class LifecycleListener extends BaseSessionEventListener { + private final List operations = new ArrayList<>(); + + @Override + public void jdbcExecuteStatementStart() { + operations.add(LifecycleOperation.STATEMENT); + } + + @Override + public void flushStart() { + operations.add(LifecycleOperation.FLUSH); + } + + @Override + public void transactionCompletion(boolean successful) { + operations.add(LifecycleOperation.TRANSACTION_COMPLETION); + } + } + + private enum LifecycleOperation { + STATEMENT, + FLUSH, + TRANSACTION_COMPLETION; + } +} diff --git a/extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/transaction/GetTransactionLifecycleTest.java b/extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/transaction/GetTransactionLifecycleTest.java new file mode 100644 index 0000000000000..125737565ac54 --- /dev/null +++ b/extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/transaction/GetTransactionLifecycleTest.java @@ -0,0 +1,59 @@ +package io.quarkus.hibernate.orm.transaction; + +import java.util.function.Function; + +import javax.enterprise.context.ApplicationScoped; +import javax.inject.Inject; +import javax.persistence.EntityManager; +import javax.persistence.EntityManagerFactory; +import javax.persistence.EntityTransaction; + +public class GetTransactionLifecycleTest extends AbstractTransactionLifecycleTest { + + @Inject + GetTransactionCRUD getTransactionCRUD; + + @Override + protected TestCRUD crud() { + return getTransactionCRUD; + } + + @Override + protected boolean expectDoubleFlush() { + // We expect double flushes in this case because EntityTransaction.commit() triggers a flush, + // and then the transaction synchronization will also trigger a flush before transaction completion. + // This may be a bug in ORM, but in any case there's nothing we can do about it. + return true; + } + + @ApplicationScoped + public static class GetTransactionCRUD extends TestCRUD { + @Inject + EntityManagerFactory entityManagerFactory; + + @Override + public ValueAndExecutionMetadata inTransaction(Function action) { + EntityManager entityManager = entityManagerFactory.createEntityManager(); + try (AutoCloseable closeable = entityManager::close) { + EntityTransaction tx = entityManager.getTransaction(); + tx.begin(); + ValueAndExecutionMetadata result; + try { + result = ValueAndExecutionMetadata.run(entityManager, action); + } catch (Exception e) { + try { + tx.rollback(); + } catch (Exception e2) { + e.addSuppressed(e2); + } + throw e; + } + tx.commit(); + return result; + } catch (Exception e) { + throw new IllegalStateException("Unexpected exception: " + e.getMessage(), e); + } + } + } + +} diff --git a/extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/transaction/SimpleEntity.java b/extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/transaction/SimpleEntity.java new file mode 100644 index 0000000000000..09c1209b553cd --- /dev/null +++ b/extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/transaction/SimpleEntity.java @@ -0,0 +1,36 @@ +package io.quarkus.hibernate.orm.transaction; + +import javax.persistence.Entity; +import javax.persistence.Id; + +@Entity +public class SimpleEntity { + + @Id + private long id; + + private String name; + + public SimpleEntity() { + } + + public SimpleEntity(String name) { + this.name = name; + } + + public long getId() { + return id; + } + + public void setId(long id) { + this.id = id; + } + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } +} diff --git a/extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/transaction/TransactionAnnotationLifecycleTest.java b/extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/transaction/TransactionAnnotationLifecycleTest.java new file mode 100644 index 0000000000000..da7eabe2504c4 --- /dev/null +++ b/extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/transaction/TransactionAnnotationLifecycleTest.java @@ -0,0 +1,65 @@ +package io.quarkus.hibernate.orm.transaction; + +import java.util.function.Function; + +import javax.enterprise.context.ApplicationScoped; +import javax.inject.Inject; +import javax.persistence.EntityManager; +import javax.transaction.Transactional; + +/** + * Check transaction lifecycle, including session flushes and the closing of the session. + */ +public class TransactionAnnotationLifecycleTest extends AbstractTransactionLifecycleTest { + + @Inject + TransactionAnnotationCRUD transactionAnnotationCRUD; + + @Override + protected TestCRUD crud() { + return transactionAnnotationCRUD; + } + + @Override + protected boolean expectDoubleFlush() { + // FIXME: We expect double flushes in this case, but that's a bug + return true; + } + + @ApplicationScoped + public static class TransactionAnnotationCRUD extends TestCRUD { + @Inject + EntityManager entityManager; + + @Override + @Transactional + public ValueAndExecutionMetadata create(long id, String name) { + return super.create(id, name); + } + + @Override + @Transactional + public ValueAndExecutionMetadata retrieve(long id) { + return super.retrieve(id); + } + + @Override + @Transactional + public ValueAndExecutionMetadata update(long id, String name) { + return super.update(id, name); + } + + @Override + @Transactional + public ValueAndExecutionMetadata delete(long id) { + return super.delete(id); + } + + @Override + public ValueAndExecutionMetadata inTransaction(Function action) { + // We should already be in a transaction + return ValueAndExecutionMetadata.run(entityManager, action); + } + } + +} diff --git a/extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/transaction/UserTransactionLifecycleTest.java b/extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/transaction/UserTransactionLifecycleTest.java new file mode 100644 index 0000000000000..bfd7ebcca6986 --- /dev/null +++ b/extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/transaction/UserTransactionLifecycleTest.java @@ -0,0 +1,65 @@ +package io.quarkus.hibernate.orm.transaction; + +import java.util.function.Function; + +import javax.enterprise.context.ApplicationScoped; +import javax.inject.Inject; +import javax.persistence.EntityManager; +import javax.transaction.HeuristicMixedException; +import javax.transaction.HeuristicRollbackException; +import javax.transaction.NotSupportedException; +import javax.transaction.RollbackException; +import javax.transaction.SystemException; +import javax.transaction.UserTransaction; + +/** + * Check transaction lifecycle, including session flushes and the closing of the session. + */ +public class UserTransactionLifecycleTest extends AbstractTransactionLifecycleTest { + + @Inject + UserTransactionCRUD userTransactionCRUD; + + @Override + protected TestCRUD crud() { + return userTransactionCRUD; + } + + @Override + protected boolean expectDoubleFlush() { + // FIXME: We expect double flushes in this case, but that's a bug + return true; + } + + @ApplicationScoped + public static class UserTransactionCRUD extends TestCRUD { + @Inject + EntityManager entityManager; + @Inject + UserTransaction userTransaction; + + @Override + public ValueAndExecutionMetadata inTransaction(Function action) { + try { + userTransaction.begin(); + ValueAndExecutionMetadata result; + try { + result = ValueAndExecutionMetadata.run(entityManager, action); + } catch (Exception e) { + try { + userTransaction.rollback(); + } catch (Exception e2) { + e.addSuppressed(e2); + } + throw e; + } + userTransaction.commit(); + return result; + } catch (NotSupportedException | SystemException | RollbackException | HeuristicMixedException + | HeuristicRollbackException e) { + throw new IllegalStateException("Unexpected exception: " + e.getMessage(), e); + } + } + } + +} From 373042f230090268c4cb12486634eb5ec14629e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yoann=20Rodi=C3=A8re?= Date: Thu, 14 Jan 2021 18:36:02 +0100 Subject: [PATCH 2/7] Test warnings when executing StoredProcedureQuery (#13273) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Yoann Rodière --- .../AbstractTransactionLifecycleTest.java | 49 ++++++++++++++++++- .../TransactionAnnotationLifecycleTest.java | 9 +++- .../UserTransactionLifecycleTest.java | 3 +- 3 files changed, 56 insertions(+), 5 deletions(-) diff --git a/extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/transaction/AbstractTransactionLifecycleTest.java b/extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/transaction/AbstractTransactionLifecycleTest.java index c7ebb5fb4a1ab..ff9843b490874 100644 --- a/extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/transaction/AbstractTransactionLifecycleTest.java +++ b/extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/transaction/AbstractTransactionLifecycleTest.java @@ -2,6 +2,9 @@ import static org.assertj.core.api.Assertions.assertThat; +import java.sql.Connection; +import java.sql.SQLException; +import java.sql.Statement; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -11,6 +14,8 @@ import java.util.logging.LogRecord; import javax.persistence.EntityManager; +import javax.persistence.ParameterMode; +import javax.persistence.StoredProcedureQuery; import org.hibernate.BaseSessionEventListener; import org.hibernate.Session; @@ -18,13 +23,17 @@ import org.hibernate.engine.spi.SharedSessionContractImplementor; import org.jboss.shrinkwrap.api.ShrinkWrap; import org.jboss.shrinkwrap.api.spec.JavaArchive; +import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; +import io.agroal.api.AgroalDataSource; +import io.quarkus.arc.Arc; import io.quarkus.test.QuarkusUnitTest; /** - * Check transaction lifecycle, including session flushes and the closing of the session. + * Check transaction lifecycle, including session flushes, the closing of the session, + * and the release of JDBC resources. */ public abstract class AbstractTransactionLifecycleTest { @@ -44,6 +53,17 @@ public abstract class AbstractTransactionLifecycleTest { .extracting(LogRecord::getMessage) // This is just to get meaningful error messages, as LogRecord doesn't have a toString() .isEmpty()); + @BeforeAll + public static void installStoredProcedure() throws SQLException { + AgroalDataSource dataSource = Arc.container().instance(AgroalDataSource.class).get(); + try (Connection conn = dataSource.getConnection()) { + try (Statement st = conn.createStatement()) { + st.execute("CREATE ALIAS " + MyStoredProcedure.NAME + + " FOR \"" + MyStoredProcedure.class.getName() + ".execute\""); + } + } + } + @Test public void testLifecycle() { long id = 1L; @@ -78,6 +98,13 @@ public void testLifecycle() { LifecycleOperation.TRANSACTION_COMPLETION); assertThat(retrieved.value).isEqualTo(UPDATED_NAME); + // See https://github.com/quarkusio/quarkus/issues/13273 + ValueAndExecutionMetadata calledStoredProcedure = crud.callStoredProcedure(id); + checkPostConditions(calledStoredProcedure, + // Strangely, calling a stored procedure isn't considered as a statement for Hibernate ORM listeners + LifecycleOperation.TRANSACTION_COMPLETION); + assertThat(calledStoredProcedure.value).isEqualTo(MyStoredProcedure.execute(id)); + ValueAndExecutionMetadata deleted = crud.delete(id); checkPostConditions(deleted, LifecycleOperation.STATEMENT, // select @@ -124,6 +151,16 @@ public ValueAndExecutionMetadata retrieve(long id) { }); } + public ValueAndExecutionMetadata callStoredProcedure(long id) { + return inTransaction(entityManager -> { + StoredProcedureQuery storedProcedure = entityManager.createStoredProcedureQuery(MyStoredProcedure.NAME); + storedProcedure.registerStoredProcedureParameter(0, Long.class, ParameterMode.IN); + storedProcedure.setParameter(0, id); + storedProcedure.execute(); + return (String) storedProcedure.getSingleResult(); + }); + } + public ValueAndExecutionMetadata update(long id, String name) { return inTransaction(entityManager -> { SimpleEntity entity = entityManager.find(SimpleEntity.class, id); @@ -188,4 +225,14 @@ private enum LifecycleOperation { FLUSH, TRANSACTION_COMPLETION; } + + public static class MyStoredProcedure { + private static final String NAME = "myStoredProc"; + private static final String RESULT_PREFIX = "StoredProcResult"; + + @SuppressWarnings("unused") + public static String execute(long id) { + return RESULT_PREFIX + id; + } + } } diff --git a/extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/transaction/TransactionAnnotationLifecycleTest.java b/extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/transaction/TransactionAnnotationLifecycleTest.java index da7eabe2504c4..c1e1c63b927da 100644 --- a/extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/transaction/TransactionAnnotationLifecycleTest.java +++ b/extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/transaction/TransactionAnnotationLifecycleTest.java @@ -22,8 +22,7 @@ protected TestCRUD crud() { @Override protected boolean expectDoubleFlush() { - // FIXME: We expect double flushes in this case, but that's a bug - return true; + return false; } @ApplicationScoped @@ -43,6 +42,12 @@ public ValueAndExecutionMetadata retrieve(long id) { return super.retrieve(id); } + @Override + @Transactional + public ValueAndExecutionMetadata callStoredProcedure(long id) { + return super.callStoredProcedure(id); + } + @Override @Transactional public ValueAndExecutionMetadata update(long id, String name) { diff --git a/extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/transaction/UserTransactionLifecycleTest.java b/extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/transaction/UserTransactionLifecycleTest.java index bfd7ebcca6986..0aa6e612f07a1 100644 --- a/extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/transaction/UserTransactionLifecycleTest.java +++ b/extensions/hibernate-orm/deployment/src/test/java/io/quarkus/hibernate/orm/transaction/UserTransactionLifecycleTest.java @@ -27,8 +27,7 @@ protected TestCRUD crud() { @Override protected boolean expectDoubleFlush() { - // FIXME: We expect double flushes in this case, but that's a bug - return true; + return false; } @ApplicationScoped From 13696e9ea4a3bfcbd5f73332cdf937d894291846 Mon Sep 17 00:00:00 2001 From: Sanne Grinovero Date: Fri, 1 May 2020 16:22:44 +0100 Subject: [PATCH 3/7] Set Hibernate ORM connection handling strategy to BEFORE_TRANSACTION_COMPLETION --- .../runtime/boot/FastBootMetadataBuilder.java | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/boot/FastBootMetadataBuilder.java b/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/boot/FastBootMetadataBuilder.java index 9aced35f01c13..f25513ba2e933 100644 --- a/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/boot/FastBootMetadataBuilder.java +++ b/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/boot/FastBootMetadataBuilder.java @@ -64,6 +64,7 @@ import org.hibernate.jpa.internal.util.LogHelper; import org.hibernate.jpa.internal.util.PersistenceUnitTransactionTypeHelper; import org.hibernate.jpa.spi.IdentifierGeneratorStrategyProvider; +import org.hibernate.resource.jdbc.spi.PhysicalConnectionHandlingMode; import org.hibernate.resource.transaction.backend.jdbc.internal.JdbcResourceLocalTransactionCoordinatorBuilderImpl; import org.hibernate.resource.transaction.backend.jta.internal.JtaTransactionCoordinatorBuilderImpl; import org.hibernate.service.Service; @@ -241,6 +242,21 @@ private MergedSettings mergeSettings(PersistenceUnitDescriptor persistenceUnit) //Agroal already does disable auto-commit, so Hibernate ORM should trust that: cfg.put(AvailableSettings.CONNECTION_PROVIDER_DISABLES_AUTOCOMMIT, Boolean.TRUE.toString()); + /** + * Set CONNECTION_HANDLING to DELAYED_ACQUISITION_AND_RELEASE_BEFORE_TRANSACTION_COMPLETION + * as it generally performs better, at no known drawbacks. + * This is a new mode in Hibernate ORM, it might become the default in the future. + * + * @see org.hibernate.resource.jdbc.spi.PhysicalConnectionHandlingMode + */ + { + final Object explicitSetting = cfg.get(AvailableSettings.CONNECTION_HANDLING); + if (explicitSetting == null) { + cfg.put(AvailableSettings.CONNECTION_HANDLING, + PhysicalConnectionHandlingMode.DELAYED_ACQUISITION_AND_RELEASE_BEFORE_TRANSACTION_COMPLETION); + } + } + if (readBooleanConfigurationValue(cfg, WRAP_RESULT_SETS)) { LOG.warn("Wrapping result sets is not supported. Setting " + WRAP_RESULT_SETS + " to false."); } From d39f754dcf3aa4d53c1fa19712f5909596242f9b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yoann=20Rodi=C3=A8re?= Date: Wed, 13 Jan 2021 11:16:27 +0100 Subject: [PATCH 4/7] Simplify connection handling setup and add a note about #7242 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DELAYED_ACQUISITION_AND_RELEASE_AFTER_STATEMENT, on top of being less efficient, also leads to resource leaks (that are caught by Agroal, but still). Signed-off-by: Yoann Rodière --- .../orm/runtime/boot/FastBootMetadataBuilder.java | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/boot/FastBootMetadataBuilder.java b/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/boot/FastBootMetadataBuilder.java index f25513ba2e933..44ea98d385395 100644 --- a/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/boot/FastBootMetadataBuilder.java +++ b/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/boot/FastBootMetadataBuilder.java @@ -242,20 +242,18 @@ private MergedSettings mergeSettings(PersistenceUnitDescriptor persistenceUnit) //Agroal already does disable auto-commit, so Hibernate ORM should trust that: cfg.put(AvailableSettings.CONNECTION_PROVIDER_DISABLES_AUTOCOMMIT, Boolean.TRUE.toString()); - /** + /* * Set CONNECTION_HANDLING to DELAYED_ACQUISITION_AND_RELEASE_BEFORE_TRANSACTION_COMPLETION * as it generally performs better, at no known drawbacks. * This is a new mode in Hibernate ORM, it might become the default in the future. * + * Note: other connection handling modes lead to leaked resources, statements in particular. + * See https://github.com/quarkusio/quarkus/issues/7242, https://github.com/quarkusio/quarkus/issues/13273 + * * @see org.hibernate.resource.jdbc.spi.PhysicalConnectionHandlingMode */ - { - final Object explicitSetting = cfg.get(AvailableSettings.CONNECTION_HANDLING); - if (explicitSetting == null) { - cfg.put(AvailableSettings.CONNECTION_HANDLING, - PhysicalConnectionHandlingMode.DELAYED_ACQUISITION_AND_RELEASE_BEFORE_TRANSACTION_COMPLETION); - } - } + cfg.putIfAbsent(AvailableSettings.CONNECTION_HANDLING, + PhysicalConnectionHandlingMode.DELAYED_ACQUISITION_AND_RELEASE_BEFORE_TRANSACTION_COMPLETION); if (readBooleanConfigurationValue(cfg, WRAP_RESULT_SETS)) { LOG.warn("Wrapping result sets is not supported. Setting " + WRAP_RESULT_SETS + " to false."); From a488c712753da60ceed81de46521e65cafd7fa11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yoann=20Rodi=C3=A8re?= Date: Tue, 12 Jan 2021 18:50:31 +0100 Subject: [PATCH 5/7] Let Hibernate ORM handle the flush/close after a transaction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We were doing that through an extra transaction synchronization, but there's no need since Hibernate ORM has built-in features to handle that. Note that without this patch: 1. Hibernate ORM flushes before transaction completion. 2. Then Hibernate ORM closes the connection wrapper. 3. Then Quarkus flushes again in its own synchronization, which usually does nothing, but in some cases (when the dirtiness of some entities cannot be known, for example when there are mutable @Converted properties) it will execute the updates again... This is problematic in itself, but it will also lead to acquiring a new connection wrapper from Agroal, which will not be closed. 4. Then when the commit happens, Agroal notices a connection wrapper was not closed and logs a warning. Signed-off-by: Yoann Rodière --- .../runtime/boot/FastBootMetadataBuilder.java | 3 +++ .../session/TransactionScopedSession.java | 19 +++++++------------ .../PrometheusMetricsRegistryTest.java | 2 +- 3 files changed, 11 insertions(+), 13 deletions(-) diff --git a/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/boot/FastBootMetadataBuilder.java b/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/boot/FastBootMetadataBuilder.java index 44ea98d385395..ab7a594f89bd2 100644 --- a/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/boot/FastBootMetadataBuilder.java +++ b/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/boot/FastBootMetadataBuilder.java @@ -255,6 +255,9 @@ private MergedSettings mergeSettings(PersistenceUnitDescriptor persistenceUnit) cfg.putIfAbsent(AvailableSettings.CONNECTION_HANDLING, PhysicalConnectionHandlingMode.DELAYED_ACQUISITION_AND_RELEASE_BEFORE_TRANSACTION_COMPLETION); + // Auto-close sessions before transaction completion, as they should be when using JTA. + cfg.putIfAbsent(AvailableSettings.AUTO_CLOSE_SESSION, "true"); + if (readBooleanConfigurationValue(cfg, WRAP_RESULT_SETS)) { LOG.warn("Wrapping result sets is not supported. Setting " + WRAP_RESULT_SETS + " to false."); } diff --git a/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/session/TransactionScopedSession.java b/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/session/TransactionScopedSession.java index 3eafb8353c051..21c89cf29d9fe 100644 --- a/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/session/TransactionScopedSession.java +++ b/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/session/TransactionScopedSession.java @@ -18,7 +18,6 @@ import javax.persistence.criteria.CriteriaUpdate; import javax.persistence.metamodel.Metamodel; import javax.transaction.Status; -import javax.transaction.Synchronization; import javax.transaction.TransactionManager; import javax.transaction.TransactionSynchronizationRegistry; @@ -86,17 +85,13 @@ SessionResult acquireSession() { Session newSession = sessionFactory.openSession(); newSession.joinTransaction(); transactionSynchronizationRegistry.putResource(sessionKey, newSession); - transactionSynchronizationRegistry.registerInterposedSynchronization(new Synchronization() { - @Override - public void beforeCompletion() { - newSession.flush(); - } - - @Override - public void afterCompletion(int i) { - newSession.close(); - } - }); + // No need to flush or close the session upon transaction completion: + // Hibernate ORM itself registers a transaction that does just that. + // See: + // - io.quarkus.hibernate.orm.runtime.boot.FastBootMetadataBuilder.mergeSettings + // - org.hibernate.resource.transaction.backend.jta.internal.JtaTransactionCoordinatorImpl.joinJtaTransaction + // - org.hibernate.internal.SessionImpl.beforeTransactionCompletion + // - org.hibernate.internal.SessionImpl.afterTransactionCompletion return new SessionResult(newSession, false, true); } else { //this will throw an exception if the request scope is not active diff --git a/integration-tests/micrometer-prometheus/src/test/java/io/quarkus/it/micrometer/prometheus/PrometheusMetricsRegistryTest.java b/integration-tests/micrometer-prometheus/src/test/java/io/quarkus/it/micrometer/prometheus/PrometheusMetricsRegistryTest.java index 6a9566de4446f..3a64d1c75226e 100644 --- a/integration-tests/micrometer-prometheus/src/test/java/io/quarkus/it/micrometer/prometheus/PrometheusMetricsRegistryTest.java +++ b/integration-tests/micrometer-prometheus/src/test/java/io/quarkus/it/micrometer/prometheus/PrometheusMetricsRegistryTest.java @@ -149,7 +149,7 @@ void testPrometheusScrapeEndpoint() { .body(containsString( "hibernate_entities_inserts_total{entityManagerFactory=\"\",env=\"test\",registry=\"prometheus\",} 3.0")) .body(containsString( - "hibernate_flushes_total{entityManagerFactory=\"\",env=\"test\",registry=\"prometheus\",} 2.0")) + "hibernate_flushes_total{entityManagerFactory=\"\",env=\"test\",registry=\"prometheus\",} 1.0")) // Annotated counters .body(not(containsString("metric_none"))) From 3756ff8adb9f2145de88528d4ef58b7e38b9419a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yoann=20Rodi=C3=A8re?= Date: Wed, 13 Jan 2021 17:17:32 +0100 Subject: [PATCH 6/7] Let the session join JTA transactions implicitly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The call to joinTransaction() is not necessary, the Session already joins the transaction when it starts. Signed-off-by: Yoann Rodière --- .../hibernate/orm/runtime/session/TransactionScopedSession.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/session/TransactionScopedSession.java b/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/session/TransactionScopedSession.java index 21c89cf29d9fe..f4046bfaee123 100644 --- a/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/session/TransactionScopedSession.java +++ b/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/session/TransactionScopedSession.java @@ -83,7 +83,7 @@ SessionResult acquireSession() { return new SessionResult(session, false, true); } Session newSession = sessionFactory.openSession(); - newSession.joinTransaction(); + // The session has automatically joined the JTA transaction when it was constructed. transactionSynchronizationRegistry.putResource(sessionKey, newSession); // No need to flush or close the session upon transaction completion: // Hibernate ORM itself registers a transaction that does just that. From d9ed7bc9a9b362cab20dc9c67559513efe9b81e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yoann=20Rodi=C3=A8re?= Date: Tue, 12 Jan 2021 19:08:54 +0100 Subject: [PATCH 7/7] Allow setting a different connection handling mode and auto-close mode for non-injected sessions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With this patch, hopefully people can safely use EntityManagerFactory.createEntityManager() and opening transactions manually? Signed-off-by: Yoann Rodière --- .../runtime/boot/FastBootMetadataBuilder.java | 3 -- .../orm/runtime/session/JTASessionOpener.java | 50 +++++++++++++++++++ .../session/TransactionScopedSession.java | 4 +- 3 files changed, 53 insertions(+), 4 deletions(-) create mode 100644 extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/session/JTASessionOpener.java diff --git a/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/boot/FastBootMetadataBuilder.java b/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/boot/FastBootMetadataBuilder.java index ab7a594f89bd2..44ea98d385395 100644 --- a/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/boot/FastBootMetadataBuilder.java +++ b/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/boot/FastBootMetadataBuilder.java @@ -255,9 +255,6 @@ private MergedSettings mergeSettings(PersistenceUnitDescriptor persistenceUnit) cfg.putIfAbsent(AvailableSettings.CONNECTION_HANDLING, PhysicalConnectionHandlingMode.DELAYED_ACQUISITION_AND_RELEASE_BEFORE_TRANSACTION_COMPLETION); - // Auto-close sessions before transaction completion, as they should be when using JTA. - cfg.putIfAbsent(AvailableSettings.AUTO_CLOSE_SESSION, "true"); - if (readBooleanConfigurationValue(cfg, WRAP_RESULT_SETS)) { LOG.warn("Wrapping result sets is not supported. Setting " + WRAP_RESULT_SETS + " to false."); } diff --git a/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/session/JTASessionOpener.java b/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/session/JTASessionOpener.java new file mode 100644 index 0000000000000..a9e22a266849e --- /dev/null +++ b/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/session/JTASessionOpener.java @@ -0,0 +1,50 @@ +package io.quarkus.hibernate.orm.runtime.session; + +import org.hibernate.FlushMode; +import org.hibernate.Session; +import org.hibernate.SessionBuilder; +import org.hibernate.SessionFactory; +import org.hibernate.context.spi.CurrentTenantIdentifierResolver; +import org.hibernate.engine.spi.SessionFactoryImplementor; +import org.hibernate.resource.jdbc.spi.PhysicalConnectionHandlingMode; + +/** + * A delegate for opening a JTA-enabled Hibernate ORM session. + *

+ * The main purpose of this class is to cache session options when possible; + * if we didn't care about caching, we could just replace any call to + */ +public class JTASessionOpener { + public static JTASessionOpener create(SessionFactory sessionFactory) { + final CurrentTenantIdentifierResolver currentTenantIdentifierResolver = sessionFactory + .unwrap(SessionFactoryImplementor.class).getCurrentTenantIdentifierResolver(); + if (currentTenantIdentifierResolver == null) { + // No tenant ID resolver: we can cache the options. + return new JTASessionOpener(sessionFactory, createOptions(sessionFactory)); + } else { + // There is a tenant ID resolver: we cannot cache the options. + return new JTASessionOpener(sessionFactory, null); + } + } + + private static SessionBuilder createOptions(SessionFactory sessionFactory) { + return sessionFactory.withOptions() + .autoClose(true) // .owner() is deprecated as well, so it looks like we need to rely on deprecated code... + .connectionHandlingMode( + PhysicalConnectionHandlingMode.DELAYED_ACQUISITION_AND_RELEASE_BEFORE_TRANSACTION_COMPLETION) + .flushMode(FlushMode.ALWAYS); + } + + private final SessionFactory sessionFactory; + private final SessionBuilder cachedOptions; + + public JTASessionOpener(SessionFactory sessionFactory, SessionBuilder cachedOptions) { + this.sessionFactory = sessionFactory; + this.cachedOptions = cachedOptions; + } + + public Session openSession() { + SessionBuilder options = cachedOptions != null ? cachedOptions : createOptions(sessionFactory); + return options.openSession(); + } +} diff --git a/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/session/TransactionScopedSession.java b/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/session/TransactionScopedSession.java index f4046bfaee123..87661331c38c9 100644 --- a/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/session/TransactionScopedSession.java +++ b/extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/session/TransactionScopedSession.java @@ -59,6 +59,7 @@ public class TransactionScopedSession implements Session { private final TransactionManager transactionManager; private final TransactionSynchronizationRegistry transactionSynchronizationRegistry; private final SessionFactory sessionFactory; + private final JTASessionOpener jtaSessionOpener; private final String unitName; private final String sessionKey; private final Instance requestScopedSessions; @@ -71,6 +72,7 @@ public TransactionScopedSession(TransactionManager transactionManager, this.transactionManager = transactionManager; this.transactionSynchronizationRegistry = transactionSynchronizationRegistry; this.sessionFactory = sessionFactory; + this.jtaSessionOpener = JTASessionOpener.create(sessionFactory); this.unitName = unitName; this.sessionKey = this.getClass().getSimpleName() + "-" + unitName; this.requestScopedSessions = requestScopedSessions; @@ -82,7 +84,7 @@ SessionResult acquireSession() { if (session != null) { return new SessionResult(session, false, true); } - Session newSession = sessionFactory.openSession(); + Session newSession = jtaSessionOpener.openSession(); // The session has automatically joined the JTA transaction when it was constructed. transactionSynchronizationRegistry.putResource(sessionKey, newSession); // No need to flush or close the session upon transaction completion: