Hibernate reactive panache - should a transaction rollback dispose the current session?

[mkouba]

Currently, a new Mutiny.Session is created per request, i.e. the ReactiveSessionProducer.createMutinySession() producer method is @RequestScoped. This bean is then used by PanacheEntity. However, if "the session is considered to be unusable after any of its methods throws an exception" - Using the reactive session. This may lead to unexpected errors if a user attempts to use a PanacheEntity e.g. after an unsuccessful flush operation...

Panache.withTransaction(...)
                    .onFailure()
                    .recoverWithUni(t -> {
                      // do something with a PanacheEntity
                     }

I wonder if a transaction rollback should result in the ReactiveSessionProducer.createMutinySession() bean destruction?

FTR there are several ways to destroy a bean explicitly. In Quarkus, you can do e.g. Arc.container().instance(Mutiny.Session.class).destroy().

[quarkus-bot[bot]]

/cc @DavideD, @FroMage, @Sanne, @gavinking, @loicmathieu

[DavideD]

Maybe I'm wrong on this but I don't think it's an issue to reuse the same session if the transaction is marked for rollback and there are no errors (@gavinking ?)

I think the problem is that Panache.withTransaction( ...) doesn't discard the current session when there is an error.
In the following example:

Panache.withTransaction(...) 
                    .onFailure()
                    .recoverWithUni(t -> {
                         return Panache.getSession().find(...); 
                     }

Panache.getSession() will return the same session that caused the exception and that's something that can cause weird errors.

Another issue comes when using @ReactiveTransactional. The example in the guide works because the session is not used during recovery.

I think it would also be helpful to add some methods to Panache to explicitly discard the current session and create a new one. Something like Panache.discardSession() and Panache.newSession() (or newSession could do both).

[gavinking]

Maybe I'm wrong on this but I don't think it's an issue to reuse the same session if the transactions is marked for rollback and there are no errors (@gavinking ?)

No, that's fine. The rule is: you must discard the session if one of its methods throws an exception.

You're allowed to use setRollbackOnly() to your heart's content.

[mkouba]

Ok, let me rephrase the question: wouldn't it be convenient/more safe to discard the session if the upstream Uni fires a failure? (Uni returned by Supplier for Panache.withTransaction() or Function for Mutiny.Session.withTransaction())

The current behavior is not very intuitive and may lead to errors that are hard to identify.

I think it would also be helpful to add some methods to Panache to explicitly discard the current session and create a new one. Something like Panache.discardSession() and Panache.newSession() (or newSession could do both).

A similar convenient method would be helpful but still a user needs to know that it exists and when exactly to call it ;-)

[DavideD]

wouldn't it be convenient/more safe to discard the session if the upstream Uni fires a failure?

Yes, isn't this the same thing I said? We need to add an onFailure then discard the session to Panache.withTransaction. I think the change will be here:

quarkus/extensions/panache/hibernate-reactive-panache/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/Panache.java

Line 39 in cb1ff41

return getSession().flatMap(session -> session.withTransaction(t -> work.get()));

A similar convenient method would be helpful but still a user needs to know that it exists and when exactly to call it ;-)

It would be for advance users and it would work as a workaround for situation that we haven't thought about yet.

[DavideD]

I think it would look like:

 return getSession().flatMap(session -> session.withTransaction(t -> work.get())).onFailure().invoke(discardSession); 

or

 return getSession().flatMap(session -> session.withTransaction(t -> work.get().onFailure().invoke(discardSession))); 

Not sure without writing real code

[DavideD]

But note that no error will be propagated if the user supplier recover from the failure. Like in the example from the guide. In that scenario, if they want to use the session, they need something like:

@ReactiveTransactional
public Uni<Void> create(Person person){
    //Here I use the persistAndFlush() shorthand method on a Panache repository to persist to database then flush the changes.
    return person.persistAndFlush()
            .onFailure(PersistenceException.class)
            .recoverWithItem(() -> {
                LOG.error("Unable to create the parameter", pe);
                // If they want to use the session again:
                return Panache.discardSession().call(this::recoverFromFailure)
            });
}

[andrekullmann]

Is a Session.clear() the same as a discardSession, isn't it?
In this case it can looks like

    public static <T> Uni<T> withTx(Supplier<Uni<T>> work) {
        return Panache.getSession().flatMap( session -> {
            return session.withTransaction( transaction -> {
                return (Uni) work.get();
            }).onFailure().invoke( () -> session.clear() );
        });
    }

[DavideD]

It's not the same because discardSession would close the current session and replace it with a completely new one.
I'm not sure if something can go wrong when using session.clear on a session that's not in a reliable state.

[Sanne]

Right, it's not the same thing. A Session#clear would clear all content of the Session and detach all entities it was managing, but there might still be registered listeners hodling a reference to this Session instance; some might be 3rd party code which we don't control.

More importantly, the Session instance is linked to an underlying connection; a clear() operation doesn't un-link this relation but the underlying connection is also identifying the transaction.

A session might have some tuning applied as well: hints, timeouts, locking strategies. This kind of internal status are not reset by clear().

[DavideD]

Note that instead of having a session.clear, the trick suggested by @mkouba should work (I haven't tested it):

Arc.container().instance(Mutiny.Session.class).destroy()

[mkouba]

It does work. I use it in my app ;-). This line basically destroys the Mutiny.Session bean instance produced by ReactiveSessionProducer#createMutinySession() and a new session is created the next time the Mutiny.Session is injected/obtained.

[andrekullmann]

In my understanding this open issue makes it implossible to use quarkus in a reactive way with hibernate. I'm a bit surprised, it looks like no one else has this problem. Is there some kind of workaround I‘ve missed?

[DavideD]

This is an issue with Panache. If you need to rollback the transaction in case of error, aside from the workaround suggested by @mkouba, you can inject the factory instead of the session:

@Inject
Mutiny.SessionFactory factory;

public Uni<Void> create(Person person){
    return factory.withTransaction( (session, tx) ->  {
         // Now you have a new session and transaction and you can control them any way you want 
    });
}

We are keeping track of this issue here: #24499

[DavideD]

I mean... we have to fix it and document it better, but I wouldn't say it's impossible to use Panache.
I will post the workaround in the issue.

[andrekullmann]

@DavideD Thank you for the clarification !!

[mkouba]

For the record - the Arc.container().instance(Mutiny.Session.class).destroy() workaround will not work in Quarkus 3.0 since Hibernate Reactive Panache does not use the session stored in the CDI request anymore (see #29761 for more details).

Something like the this can be used instead:

// ...
.onFailure()
.recoverWithUni(t -> 
   Panache.getSession()
        .chain(s -> s.close()) // close the current session
        .chain(v -> Panache.withSession(() -> doSomeRecoveryWithNewSession())
);