Connection handling fixes #14305

yrodiere · 2021-01-14T18:04:06Z

I ended up keeping support for getTransaction() for now, since it still works. We can open another pull request if we want to drop it.

Opening as draft because:

The build is expected to fail for now; we need to upgrade to a version of ORM that includes HHH-14326 + HHH-14404 Connection closing fixes hibernate/hibernate-orm#3693 first.
We need some additional testing as a previous version of this patch used to trigger deadlocks in the database. The fixes in ORM may solve these deadlocks, but that's absolutely not certain since we haven't found the exact cause yet. @johnaohara is trying to reproduce the deadlocks.

Sanne · 2021-01-15T15:08:15Z

...te-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/session/JTASessionOptions.java

+        SessionBuilder<?> options = 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)


Could you explain to me again why these options have to be set again here?

Because managed, transaction-scoped sessions have different requirements from user-created sessions (direct calls to createEntityManager()) with manual transaction management (begin/commit).

Most important is the auto-close on transaction completion, which obviously only makes sense for transaction-scoped sessions. When a user calls .createEntityManager() manually, you'd expect them to close the EM manually.

For other settings, it's really about backward compatibility. From what I can see in FastBootMetadataBuilder we still allow custom settings for all sorts of properties (from persistence.xml?), and while other connection handling modes could make sense for manually-created entity managers, I don't think they would for transaction-scoped sessions.
This solution allows us to force these settings for transaction-scoped sessions, while preserving the ability to override them for manually-created entity managers.

To sum up, we could limit this to only setting autoClose(true) here, and assume that people setting the connection handling mode and other related settings manually know what they are doing.

yrodiere · 2021-02-22T07:56:24Z

@johnaohara gave the green light for merging this. I just rebased my branch. @Sanne , as soon as you're good with my answer to your review, we can go ahead and merge :)

Thanks to @johnaohara and @barreiro for looking into this!

Signed-off-by: Yoann Rodière <yoann@hibernate.org>

…COMPLETION

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 <yoann@hibernate.org>

yrodiere · 2021-02-22T14:45:27Z

There were a few problems with multi-tenancy, so I switched to a safer (and, I think, cleaner) approach in the last commit. It should be good now.

@converted

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 <yoann@hibernate.org>

The call to joinTransaction() is not necessary, the Session already joins the transaction when it starts. Signed-off-by: Yoann Rodière <yoann@hibernate.org>

…e for non-injected sessions With this patch, hopefully people can safely use EntityManagerFactory.createEntityManager() and opening transactions manually? Signed-off-by: Yoann Rodière <yoann@hibernate.org>

yrodiere · 2021-02-23T13:45:54Z

Fixed another test, apparently the metrics module was expecting two flushes for a single transaction. That's actually a problem that I fixed in this PR: we now correctly only flush once. I changed the test so that it passes.

Relevant commit: Let Hibernate ORM handle the flush/close after a transaction.

Sanne · 2021-02-23T15:08:20Z

Fixed another test, apparently the metrics module was expecting two flushes for a single transaction. That's actually a problem that I fixed in this PR: we now correctly only flush once

Very nice!

Sanne · 2021-02-23T17:31:14Z

...orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/boot/FastBootMetadataBuilder.java

-            }
-        }
+        cfg.putIfAbsent(AvailableSettings.CONNECTION_HANDLING,
+                PhysicalConnectionHandlingMode.DELAYED_ACQUISITION_AND_RELEASE_BEFORE_TRANSACTION_COMPLETION);


N.B. that semantics is not the same: the key can be defined while the value is set to null. No? Or is the change intentional?

If it's a problem here, it's probably also a problem just above, where we use cfg.containsKey to check whether a property is defined or not.

If we're going to worry about this, I think what we need to ask is: how would we end up in a situation where a property is set, but its value is null? From what I understand the only way for properties to be pre-set here is from a persistence.xml. I'm not sure if you can set something to null from persistence.xml?

Sanne · 2021-02-23T22:10:33Z

I'm not worried about it. I only wondered if you were aware of the subtle change. And I agree it probably doesn't matter in this case, as we only want to support the option you're setting. Perhaps we should enforce it, and use the containsKey method to trigger a warning in case it was set. FYI there's various undocumented ways in which this could get overridden, e.g. hibernate.properties resources, environment variables, and programmatically via the buildItem SPI. But while backdoors are possible for exceptional cases they are not meant to be supported. No need to change this, I just wanted to point out the subtlety for awareness.

…

On Tue, 23 Feb 2021, 18:08 Yoann Rodière, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In extensions/hibernate-orm/runtime/src/main/java/io/quarkus/hibernate/orm/runtime/boot/FastBootMetadataBuilder.java <#14305 (comment)>: > * @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 it's a problem here, it's probably also a problem just above, where we use cfg.containsKey to check whether a property is defined or not. If we're going to worry about this, I think what we need to ask is: how would we end up in a situation where a property is set, but its value is null? From what I understand the only way for properties to be pre-set here is from a persistence.xml. I'm not sure if you can set something to null from persistence.xml? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14305 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAKMTIX4MJX6LCGUCPS5YDTAPVKBANCNFSM4WCZ77HA> .

Sanne

Great work, thanks!

ghost added area/hibernate-orm Hibernate ORM area/persistence OBSOLETE, DO NOT USE labels Jan 14, 2021

yrodiere mentioned this pull request Jan 14, 2021

Connection handling fixes #14304

Closed

Sanne reviewed Jan 15, 2021

View reviewed changes

yrodiere force-pushed the i7242 branch from 746b36e to 9c82b0d Compare January 21, 2021 14:47

yrodiere force-pushed the i7242 branch 2 times, most recently from 6d6e358 to 96c4792 Compare February 10, 2021 15:33

yrodiere force-pushed the i7242 branch from 96c4792 to 08bcaf3 Compare February 22, 2021 07:54

yrodiere marked this pull request as ready for review February 22, 2021 07:54

yrodiere force-pushed the i7242 branch 2 times, most recently from 2082e16 to 53f8587 Compare February 22, 2021 12:35

yrodiere and others added 4 commits February 22, 2021 14:50

Test the transaction lifecycle (session flushes, session closing, ...)

1deece0

Signed-off-by: Yoann Rodière <yoann@hibernate.org>

Test warnings when executing StoredProcedureQuery (quarkusio#13273)

373042f

Signed-off-by: Yoann Rodière <yoann@hibernate.org>

Set Hibernate ORM connection handling strategy to BEFORE_TRANSACTION_…

13696e9

…COMPLETION

Simplify connection handling setup and add a note about quarkusio#7242

d39f754

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 <yoann@hibernate.org>

yrodiere force-pushed the i7242 branch 2 times, most recently from 7c4accc to fafd4b1 Compare February 22, 2021 14:32

yrodiere added 3 commits February 23, 2021 12:26

Let the session join JTA transactions implicitly

3756ff8

The call to joinTransaction() is not necessary, the Session already joins the transaction when it starts. Signed-off-by: Yoann Rodière <yoann@hibernate.org>

Allow setting a different connection handling mode and auto-close mod…

d9ed7bc

…e for non-injected sessions With this patch, hopefully people can safely use EntityManagerFactory.createEntityManager() and opening transactions manually? Signed-off-by: Yoann Rodière <yoann@hibernate.org>

yrodiere force-pushed the i7242 branch from fafd4b1 to d9ed7bc Compare February 23, 2021 11:28

Sanne reviewed Feb 23, 2021

View reviewed changes

Sanne added the triage/qe? Issue could use a quality focused review to further harden it label Feb 25, 2021

Sanne approved these changes Feb 25, 2021

View reviewed changes

Sanne added this to the 1.13 - master milestone Feb 25, 2021

Sanne merged commit 4ec0ff9 into quarkusio:master Feb 25, 2021

yrodiere mentioned this pull request Feb 25, 2021

Warning JDBC resources leaked when executing StoredProcedureQuery #13273

Closed

Sanne mentioned this pull request Mar 9, 2021

Panache Data REST: Update method requires at least two connections #15369

Closed

yrodiere deleted the i7242 branch May 17, 2021 11:15

yrodiere mentioned this pull request Jul 28, 2022

Statement leak warning on start since 2.11.0.Final #26995

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Connection handling fixes #14305

Connection handling fixes #14305

yrodiere commented Jan 14, 2021

Sanne Jan 15, 2021

yrodiere Jan 18, 2021 •

edited

Loading

yrodiere commented Feb 22, 2021

yrodiere commented Feb 22, 2021 •

edited

Loading

yrodiere commented Feb 23, 2021

Sanne commented Feb 23, 2021

Sanne Feb 23, 2021

yrodiere Feb 23, 2021

Sanne commented Feb 23, 2021 via email

Sanne left a comment

Connection handling fixes #14305

Connection handling fixes #14305

Conversation

yrodiere commented Jan 14, 2021

Sanne Jan 15, 2021

Choose a reason for hiding this comment

yrodiere Jan 18, 2021 • edited Loading

Choose a reason for hiding this comment

yrodiere commented Feb 22, 2021

yrodiere commented Feb 22, 2021 • edited Loading

yrodiere commented Feb 23, 2021

Sanne commented Feb 23, 2021

Sanne Feb 23, 2021

Choose a reason for hiding this comment

yrodiere Feb 23, 2021

Choose a reason for hiding this comment

Sanne commented Feb 23, 2021 via email

Sanne left a comment

Choose a reason for hiding this comment

yrodiere Jan 18, 2021 •

edited

Loading

yrodiere commented Feb 22, 2021 •

edited

Loading