-
-
Notifications
You must be signed in to change notification settings - Fork 101
Description
Hello again! We've been using Hibernate Reactive in prod for a while and there's a common mistake I would like to prevent when using Reactive DAO's.
We're writing DAO's much like you would in blocking code, here is an example DAO method:
/**
* List all entities. Force providing max results to avoid perf problems
*
* @return list of entities
*/
@CheckReturnValue
public Uni<List<E>> list(final int maxResults) {
Preconditions.checkArgument(
maxResults > 0, "maxResults must be more than zero. Value was %s", maxResults);
return sessionFactory.withSession(
session -> {
final var query = sessionFactory.getCriteriaBuilder().createQuery(cls);
// need root for query to be valid
query.from(this.cls);
return session.createQuery(query).setMaxResults(maxResults).getResultList();
});
}
A very common problem with this pattern is accidentally creating another Session because you have multiple reactive streams. And related, accidentally creating a second Reactive Session in a different thread because context wasn't propagated when using adapters between Mutiny/CompletableFuture/RxJava. This is actually detected and logged by Hibernate Reactive already, but at a Debug level and not exposed to the user API.
// EXISTING CODE MutinySessionFactoryImpl.withSession()
@Override
public <T> Uni<T> withSession(Function<Mutiny.Session, Uni<T>> work) {
Objects.requireNonNull( work, "parameter 'work' is required" );
Mutiny.Session current = context.get( contextKeyForSession );
if ( current != null && current.isOpen() ) {
LOG.debug( "Reusing existing open Mutiny.Session which was found in the current Vert.x context" );
return work.apply( current );
}
else {
LOG.debug( "No existing open Mutiny.Session was found in the current Vert.x context: opening a new instance" );
return withSession( openSession(), work, contextKeyForSession );
}
}
Can we expose this logic so I can warn or something when calling DAO methods without a session in progress?
Optional<Mutiny.Session> getCurrentSession() {
Mutiny.Session current = context.get( contextKeyForSession );
if ( current != null && current.isOpen() ) {
return Optional.of(current);
}
else {
return Optional.empty();
}
}
Another option is exposing a set of withSession()/withTransaction() variants on SessionFactory
that blow up if a session isn't in progress... maybe named withExistingSession()
. This my plan for our code-base since we already wrap SessionFactory to hide Open*** variants because lack of automatic flush has bitten us so many times.
My goal is preventing implicit Session creation while transparently using an existing one in our DAO's. Because IMO it's too easy to accidentally drop your Session out of context using streams or threading improperly, and end up with utterly intractable and random
IllegalStateException: HR000069: Detected use of the reactive Session from a different Thread
Activity
DavideD commentedon Aug 9, 2022
I need to think about it, but you can get the context via the Implementor interface:
noobgramming commentedon Aug 11, 2022
Hello David, I've been fiddling with this on my end. The ability to get current session remains useful, but I'm starting to doubt my other suggestions.
I found a more effective way to fix
IllegalStateException: HR000069: Detected use of the reactive Session from a different Thread
although I'm unsure if it can be done in a general way without causing unacceptable performance impact (I put our implementation behind a run-time modifiable config option)I realized the main problem with Hibernate's existing error message is that it doesn't preserve the original stack trace. When you get
HR000069
, it can be coming from anywhere in your application as the provided trace doesn't contain any of the user's stack frames.So for our SessionFactory wrapper, I'm now saving off a
new RuntimeException()
along with current threadId and name as the value in a concurrent map with weak keys ofMutiny.Session
whenever the Session is used. Like this:This allows me to lookup which thread the Session was created on each time it's used, and more importantly, the stored RuntimeException allows me to re-hydrate the original stack trace. Finally, the weak keys ensure I don't create a memory leak holding onto Sessions no longer in use. Usage looks something like this
The main problem with my implementation is that creating a
new RuntimeException()
and stuffing it into the map every time a Session is used is very inefficient. I'm not clever or familiar enough with Hibernate to find a solution to this.Maybe there is a similar but more performant way to improve the original
HR000069
error messages that led me down this rabbithole?DavideD commentedon Aug 12, 2022
Ah, we didn't notice that. Good point!
When using the Mutiny API, we create a Uni that will eventually be executed at subscription and so, in case of error, it doesn't contain the caller of the session method causing the error.
For reference, this is the stacktrace when running InternalStateAssertionsTest#testFindWithStage:
And this is the stack trace when running InternalStateAssertionsTest#testFindWithMutiny:
And it doesn't contain any reference to
InternalStateAssertionsTest
.[hibernate#1376] Keep track of the caller in case of exception
[hibernate#1376] Keep track of the caller in case of exception
DavideD commentedon Aug 13, 2022
I don't think I would keep a map around to fix this issue.
You can create a class that wraps the session and every time you call one of the method in it, keep track of possible errors.
You can see an example - it might become a future fix - here: https://github.com/DavideD/hibernate-reactive/blob/stacktrace/hibernate-reactive-core/src/main/java/org/hibernate/reactive/mutiny/impl/MutinySessionImpl.java#L57
[-]Expose check for Hibernate Session already in progress or otherwise prevent implicit Session creation? [/-][+]Improve stack trace for Session used across threads detection error HR000069 when using Mutiny[/+]gavinking commentedon Sep 6, 2022
@noobgramming not sure what you mean by this; different streams can't share
Session
s, that's against the rules.noobgramming commentedon Sep 6, 2022
@gavinking I know, but it hasn't stopped people from doing it :( . At least in our projects
paomian commentedon Nov 17, 2022
I do't want to share
Session
In my situation. I will send email to an address which query from db. Unfortunately event loop of query email is different with send email.
consider code
@ReactiveTransactional
will check event loop which used by methodgagaga
butsendInviteEmail
is wrapped aws ses service. aws ses service use this own event loop. aws ses return a CompletableFuture. I will get same error when i create Uni byUni.createFrom().completionStage(future)
. Uni will use previous step executor I guess.Now i can run blocking CompletableFuture by
get
and generate a Uni byitem(result)
,but it will be blocking work thread.So i try to use
emitOn
changesendInviteEmail
event loop. but i do not know which executor to chose. Any one can help me?DavideD commentedon Nov 17, 2022
I'm not sure I understand what's going on, but can't you delay the creation of the session? it doesn't seem like you are using it in
gagaga
and I don't know what's happening insendInviteEmail
.@ReactiveTransactional
just wraps the method call usingPanache.withTransaction
. So you could try removing the annotation and usePanache.withTransaction
when you actually need it.It would help if you could create a project or test so that we can see the error.
Also, this issue is about improving the stack trace, I think you are asking for something different.
12 remaining items