-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Connection handling fixes #14305
Connection handling fixes #14305
Conversation
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain to me again why these options have to be set again here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
6d6e358
to
96c4792
Compare
@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! |
2082e16
to
53f8587
Compare
Signed-off-by: Yoann Rodière <yoann@hibernate.org>
Signed-off-by: Yoann Rodière <yoann@hibernate.org>
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>
7c4accc
to
fafd4b1
Compare
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. |
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>
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: |
Very nice! |
} | ||
} | ||
cfg.putIfAbsent(AvailableSettings.CONNECTION_HANDLING, | ||
PhysicalConnectionHandlingMode.DELAYED_ACQUISITION_AND_RELEASE_BEFORE_TRANSACTION_COMPLETION); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
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>
.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, thanks!
Fixes #7242, #13273.
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: