quarkus-hibernate-reactive does not make StatelessSession available for injection

[gavinking]

Description

The regular quarkus-hibernate-orm extension makes a Hibernate StatelessSession available for injection, which means that Jakarta Data repositories can be easily injected in user code.

But quarkus-hibernate-reactive does not do this for Mutiny.StatelessSession (nor even for Uni<Mutiny.StatelessSession>) and this means that injection of Jakarta Data repositories is an almost impossible task for an unsophisticated user.

Yes, it's possible to make this work by writing your own @Producer method on a @RequestScoped bean, and also adding a custom @Interceptor to make sure everything gets cleaned up properly, but this was a task that took me more than an hour, not counting the time I spent fixing tangential issues I ran into along the way.

Implementation ideas

@FroMage already has some discussion of this in the context of Panache here: #46091, but this is actually more general and affects people who aren't using Panache.

[quarkus-bot]

/cc @DavideD (hibernate-reactive), @Ladicek (arc), @manovotn (arc), @mkouba (arc)

[DavideD]

At some point it was possible to inject Uni<Mutiny.Session> or Mutiny.Session, but we decided to remove the feature because of problems that I can't remember on top of my head. I think it was something related to have connection leaks, error handling in case of exceptions, and we also sort of agreed that using SessionFactory.withTransaction was better because the scope of the session was clearer.

(I'm on PTO, so I didn't look too in depth into this)

Maybe @mkouba, or @Sanne remember?

[mkouba]

The HR Mutiny.Session bean was removed after the big refactoring of HR Panache. We had many problems with concurrency access, session leaks, context propagation, etc. Note that at that time we did not store the CDI request context in the Vertx duplicated context (it was threadlocal-based only). This should not be a problem anymore (in most use cases; not so much for some creative MP Context Propagation use cases).

Yes, it's possible to make this work by writing your own @Producer method on a @RequestScoped bean, and also adding a custom @interceptor to make sure everything gets cleaned up properly, but this was a task that took me more than an hour, not counting the time I spent fixing tangential issues I ran into along the way.

@gavinking Why a custom interceptor? @Disposes was not enough? Also what exactly needs to be "cleaned up properly"?

[gavinking]

We had many problems with concurrency access, session leaks, context propagation, etc.

Removing the functionality does not solve the problem. It merely pushes the problem somewhere else where it's even harder to solve. Most likely into user code.

Note that at that time we did not store the CDI request context in the Vertx duplicated context (it was threadlocal-based only). This should not be a problem anymore (in most use cases; not so much for some creative MP Context Propagation use cases).

OK, great, so I think what you're saying is that we can put this functionality back in now?

Why a custom interceptor? @Disposes was not enough? Also what exactly needs to be "cleaned up properly"?

The StatelessSession must be close()d after use.

@Disposes does not work because creation of a reactiveStatelessSession returns Uni<StatelessSession>, and that Uni is viral, meaning that any access to the StatelessSession, even close() must occur on a reactive stream. I don't think ArC knows what to do with a @Disposes method which returns Uni. I would be very happy to be wrong about that.

[gavinking]

Note that the best way I found to do this right now is the following:

@InterceptorBinding
@Target({ TYPE, METHOD })
@Retention(RUNTIME)
public @interface WithSession {}

@RequestScoped
public class RequestScopedSession {
    private int reentrancy = 0;
    private Mutiny.StatelessSession session;
    public void setSession(Mutiny.StatelessSession session) {
        if (session == null) {
            reentrancy--;
            if ( reentrancy == 0 ) {
                this.session = null;
            }
        }
        else {
            assert reentrancy == 0 
                || session == this.session;
            if ( reentrancy == 0 ) {
                this.session = session;
            }
            reentrancy++;
        }
    }
    @Produces @RequestScoped
    public Uni<Mutiny.StatelessSession> uniOfSession() {
        if (session == null) {
            throw new IllegalStateException("no session");
        }
        return Uni.createFrom().item(session);
    }
    @Produces @RequestScoped
    public Mutiny.StatelessSession session() {
        if (session == null) {
            throw new IllegalStateException("no session");
        }
        return session;
    }
}

@WithSession @Interceptor
public class WithSessionInterceptor {
    @Inject RequestScopedSession requestScopedSession;
    @Inject Mutiny.SessionFactory factory;
    @AroundInvoke
    public Object withSession(InvocationContext invocationContext) throws Exception {
        if ( invocationContext.getMethod().getReturnType().equals(Uni.class) ) {
            return factory.withStatelessTransaction(session -> {
                requestScopedSession.setSession(session);
                try {
                    Object result = invocationContext.proceed();
                    requestScopedSession.setSession(null);
                    return (Uni<?>) result;
                } catch (Exception e) {
                    throw new RuntimeException(e);
                }
            });
        }
        else {
            return invocationContext.proceed();
        }
    }
}

This is really a lot to expect a user to come up with on their own, and minor perturbations to this solution are likely to be broken.

Also I guess this doesn't work in native:

invocationContext.getMethod().getReturnType().equals(Uni.class)

[mkouba]

We had many problems with concurrency access, session leaks, context propagation, etc.

Removing the functionality does not solve the problem. It merely pushes the problem somewhere else where it's even harder to solve. Most likely into user code.

Well, we didn't remove the functionality but replaced with a more idiomatic approach. For pure Hibernate Reactive a Mutiny.SessionFactory should be injected instead and a new session is created by the user (at least I do remember that @DavideD confirmed this workflow back then ;-); for HR Panache we added some reasonable defaults, plus annotations like @WithSesion, @WithTransaction, etc.

Note that at that time we did not store the CDI request context in the Vertx duplicated context (it was threadlocal-based only). This should not be a problem anymore (in most use cases; not so much for some creative MP Context Propagation use cases).

OK, great, so I think what you're saying is that we can put this functionality back in now?

Not really. The problem with session destruction still remains, note that all CDI APIs are blocking.

Why a custom interceptor? @Disposes was not enough? Also what exactly needs to be "cleaned up properly"?

The StatelessSession must be close()d after use.

OK.

@Disposes does not work because creation of a reactiveStatelessSession returns Uni<StatelessSession>, and that Uni is viral, meaning that any access to the StatelessSession, even close() must occur on a reactive stream. I don't think ArC knows what to do with a @Disposes method which returns Uni. I would be very happy to be wrong about that.

You're right. ArC does not care.

[mkouba]

Note that the best way I found to do this right now is the following:

Where do you close the session?

BTW your solution seems to be similar to the io.quarkus.hibernate.reactive.panache.common.runtime.WithSessionInterceptor from HR Panache, except that there a session is stored directly in the Vertx duplicated context.

[DavideD]

Removing the functionality does not solve the problem. It merely pushes the problem somewhere else where it's even harder to solve. Most likely into user code.

In this case, it really solves the problems. Because the issues weren't cause by code written by the user, but the way all the different extensions were working together.

Well, we didn't remove the functionality but replaced with a more idiomatic approach. For pure Hibernate Reactive a Mutiny.SessionFactory should be injected instead and a new session is created by the user (at least I do remember that @DavideD confirmed this workflow back then ;-); for HR Panache we added some reasonable defaults, plus annotations like @WithSesion, @WithTransaction, etc.

Yes, and I think it worked pretty well, as far as I can tell.

Now, I'm not against injecting the session per se. A lot of things have changed since then. But it's a bit unclear to me why we need it.

[FroMage]

Note that for the Panache 2 branch, I do have a commit introducing @WithStatelessSession in my branch.

[gavinking]

Where do you close the session?

@mkouba It's closed automatically by factory.withStatelessTransaction(), of course.

[gavinking]

@DavideD

In this case, it really solves the problems. Because the issues weren't cause by code written by the user, but the way all the different extensions were working together.

It doesn't solve the problem, because it forces the user to have to write the same sort of bug-prone code, but without the knowledge that we have.

The proof of this is that I had to write the code when I tried to use Jakarta Data reactive repositories in Quarkus.