Skip to content

Conversation

@JEONGJAEIK
Copy link

@JEONGJAEIK JEONGJAEIK commented Dec 18, 2025

This PR fixes a physical connection leak that occurs in OSIV-like scenarios when concurrent access to a session
causes an exception during session close.

Problem

In OSIV (Open Session In View) environments, when async threads access lazy-loaded proxies while the main thread
closes the session, a ConcurrentModificationException can occur in LogicalConnectionManagedImpl.close().

When the exception happens during releaseResources(), the method exits early and never calls
releaseConnectionIfNeeded(), leaving the physical JDBC connection unreleased in the pool, eventually causing
connection pool exhaustion.

Solution

Wrap releaseResources() in a try-catch block to ensure releaseConnectionIfNeeded() is always executed, even
when concurrent modification exceptions occur.

Changes

  • Modified LogicalConnectionManagedImpl.close() to handle exceptions during resource release
  • Added error logging in LogicalConnectionLogging for connection release failures
  • Added ConcurrentCloseConnectionLeakTest to reproduce the OSIV-like concurrent scenario and verify the fix

Testing

The new test simulates an OSIV-like scenario with:

  • 30 parallel sessions with lazy-loaded associations
  • Async threads attempting to access lazy proxies after transaction commit
  • Main thread closing the session concurrently
  • Verification that physical connections are properly released despite ConcurrentModificationException

The test verifies:

  1. Concurrent modification exceptions are logged appropriately
  2. Physical connections are released to the pool despite exceptions
  3. No connection leaks occur (active connections = 0 after test)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.


https://hibernate.atlassian.net/browse/HHH-20002

@gavinking
Copy link
Member

concurrent access to a session causes an exception during session close

if this is a way to detect that there is concurrent access to the session going on, then the correct response to that would be to blow up with a big error telling the programmer they are doing something very wrong.

@gavinking
Copy link
Member

concurrent access to a session causes an exception during session close

if this is a way to detect that there is concurrent access to the session going on, then the correct response to that would be to blow up with a big error telling the programmer they are doing something very wrong.

Or can it actually also occur in a different way?

@gavinking
Copy link
Member

gavinking commented Dec 18, 2025

I actually don't understand what behavior is being claimed here.

If ResourceRegistry.releaseResources() throws an exception due to corruption of its internal data structure (a CCME or something like that, I suppose) then it looks like that exception propagates. I don't see us catching that exception anywhere.

Please don't tell me that you are:

  1. not only using the session from concurrent threads in an unsynchronized way, but
  2. also *catching and swallowing exceptions that occur from Session.close().

Is that the scenario here?

@JEONGJAEIK
Copy link
Author

Thanks for the questions — let me clarify the scenario.

This is not about catching or swallowing exceptions from Session.close().
Any exception thrown from ResourceRegistry.releaseResources() is still propagated
to the caller; the change only ensures that releaseConnectionIfNeeded() is executed
in a finally block so that the physical JDBC connection is not leaked.

The concurrent scenario being tested is not intentional unsynchronized access
to the same Session by user code. Rather, it occurs at the OSIV boundary, where
a proxy holding a session reference may trigger lazy initialization in an async
thread while the main request thread is concurrently closing the session.

In this case, an exception during resource cleanup can currently prevent the
physical connection from being released, leading to pool exhaustion.
The intention of this change is to avoid leaking the JDBC connection, not to
mask session misuse or allow continued operation after failure.

@gavinking
Copy link
Member

You are receiving an error notifying you that there is a very severe programming error in your code, and that you must fix your code. Cleaning up a leaked connection is the absolute least of your problems here.

@gavinking
Copy link
Member

I'm not saying we can't do the connection closing in a finally. I don't see any particular problem with that.

But the fact that you're concerned about a leaked connection in a scenario where you're using Hibernate in a completely broken and illegal way is worrying me a lot more.

@gavinking
Copy link
Member

I would much rather that you run out of connections and are forced to fix your broken session handling than that we "fix" this and you take that as permission to leave your program in its current broken state.

@JEONGJAEIK
Copy link
Author

Understood. Thanks for the clarification — that makes sense.

@JEONGJAEIK
Copy link
Author

I would much rather that you run out of connections and are forced to fix your broken session handling than that we "fix" this and you take that as permission to leave your program in its current broken state.

Understood, agreed.

One minor observability thought only: would a WARN when releaseResources() fails help make this failure mode more explicit, without changing behavior?

@gavinking
Copy link
Member

would a WARN when releaseResources() fails help make this failure mode more explicit, without changing behavior?

I believe our dogma is that "log and then rethrow" is an antipattern. You report the problem one way or the other, not both.

Comment on lines +105 to +112
CompletableFuture<Void> asyncTask = CompletableFuture.runAsync(() -> {
try {
Thread.sleep(ASYNC_DELAY_MS);
String childName = parent.getChild().getName();
}
catch (Exception ignored) {
}
}, asyncExecutor);

Check notice

Code scanning / CodeQL

Unread local variable Note test

Variable 'CompletableFuture asyncTask' is never read.
CompletableFuture<Void> asyncTask = CompletableFuture.runAsync(() -> {
try {
Thread.sleep(ASYNC_DELAY_MS);
String childName = parent.getChild().getName();

Check notice

Code scanning / CodeQL

Unread local variable Note test

Variable 'String childName' is never read.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants