hibernate-orm icon indicating copy to clipboard operation
hibernate-orm copied to clipboard

Leak of connections - draft fix for Quarkus #27166

Open Sanne opened this issue 2 years ago • 3 comments

See also https://github.com/quarkusio/quarkus/issues/27166 ; leaving it here as a draft for now as it's a very late, need to discuss with the ORM team what fix we actually want here.

Also I need to port the tests and verify applicability to main.

Also wondering: allowLoadOutsideTransaction isn't being checked when the Session owned by the interceptor is open and connected already. Is that intentional? I suppose it's plausible that if the Session was intentionally opened w/o a transaction then we want to be more lenient and allow loading a non-trivial object graph, even if this wasn't explicitly turned on?

For reference, the javadoc in main for this option:

/**
 * Allows a detached proxy or lazy collection to be fetched even when not
 * associated with an open persistence context, by creating a temporary
 * persistence context when the proxy or collection is accessed. This
 * behavior is not recommended, since it can easily break transaction
 * isolation or lead to data aliasing. It is therefore disabled by default.
 *
 * @see org.hibernate.boot.SessionFactoryBuilder#applyLazyInitializationOutsideTransaction(boolean)
 */
String ENABLE_LAZY_LOAD_NO_TRANS = "hibernate.enable_lazy_load_no_trans";

The wording seems to refer to the persistence context; the name of the property seem to focus on the transaction though.

Sanne avatar Aug 31 '22 22:08 Sanne

Thanks for your pull request!

This pull request does not follow the contribution rules. Could you have a look?

❌ All commit messages should start with a JIRA issue key matching pattern HHH-\d+     ↳ Offending commits: [8f3e4e502954a691faaf95675bcff19e3e4d4c30]

› This message was automatically generated.

Thanks @beikov , I'll check that.

After a night of sleep I also concluded that wondering about whether allowLoadOutsideTransaction is not being respected is a bit orthogonal to the issue at hand, I'll keep that as a possibly subsequent task, let's focus on fixing the leak first.

Sanne avatar Sep 01 '22 08:09 Sanne

So, well it's actually not leaking as (of course) we close it all on Session#close. It's more about a matter of timing: when exactly this should be closed.

I'm wondering now why this patch seems to fix the issue in the reproducer of the Quarkus issue though; possibly timing of close events having an impact on when we reach the connection pool limit?

Sanne avatar Sep 01 '22 11:09 Sanne

Closing as it wasn't a leak after all.

Sanne avatar Oct 27 '22 11:10 Sanne