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: https://github.com/quarkusio/quarkus/issues/46091, but this is actually more general and affects people who aren't using Panache.

[quarkus-bot[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.

[gavinking]

@mkouba

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.

OK, so in fact you didn't remove the functionality.

What you actually did was you moved the functionality to the Panache module, where it can't be used be people who aren't using Panache.

I'm completely unclear on what advantage is offered by moving things to Panache. If this code was so bug-prone, why were the bugs OK for Panache users?

The problem with session destruction still remains

There's no "problem" with session destruction, as is demonstrated in the code I already posted above.

[gavinking]

PS there's a very very tiny bug in the code I posted days ago regarding reentrancy but the fix is very trivial. Will edit the code when I get back to my laptop.

[gavinking]

there's a very very tiny bug in the code

Fixed.

There's also a problem if you want to start and commit a series of multiple transactions/sessions in the same request. Fixing that one is also straightforward using a proxy, but I'm not going to do that here.

[gavinking]

@DavideD

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.

I missed this question. And the answer is right there in the issue description:

injection of Jakarta Data repositories is an almost impossible task for an unsophisticated user.

Jakarta Data repositories are injected. Therefore, we need a way for them to get access to a contextual session. Now, of course one could contemplate various implementations, but by far the simplest way to solve the problem is to make the session available for injection via CDI.

[mkouba]

There's no "problem" with session destruction, as is demonstrated in the code I already posted above.

@gavinking So I looked at your solution and it's very different from the original injectable Mutiny.Session where a session was stored in the request context and destroyed when the request context was destroyed (hence the problem with destruction because of sync CDI API).

The WithSessionInterceptor is a mandatory part of the solution and the session is not bound to the request context but to the outermost invocation of a method annotated with @WithSession, i.e. when this invocation completes the session is not available for injection.

I think that this would work, unless you use the injected Mutiny.Session lazily in the pipeline... because when someone subscribes to the returned Uni the injected proxy would not see the RequestScopedSession#session anymore.

When I say "lazily in the pipeline" I mean something like:

class MyBean {

   @Inject // -> injected proxy that reads RequestScopedSession#session
   Mutiny.Session session;

   @WithSession
   Uni<Void> doSomething() {
      return Uni.createFrom().item(() -> {
         session.loadSomething(); // this supplier is executed when someone subscribes to the returned Uni
      }).replaceWithVoid();
   }

}

[FroMage]

I'm not sure if we should make Mutiny.Session available for injection. Perhaps Uni<Mutiny.Session> is enough?

In any case, we should probably move SessionOperations, @WithSession, @WithTransaction to Quarkus-HR and untie it from HR/Panache, no?

[gavinking]

I think that this would work, unless you use the injected Mutiny.Session lazily in the pipeline... because when someone subscribes to the returned Uni the injected proxy would not see the RequestScopedSession#session anymore.

Ummmmm, indeed, that's a good point I had not thought of.

~So you're right, we do need to make sure that the reactive session, along with all repository beans are always instantiated eagerly, and not lazily on access.~

~Don't we have some sort of annotation for that these days? I thought I saw something like that floating around. @Eager, or whatever. Something instantiated when the context is created.~

~Probably it's enough if they observe the @Initialized event for the request scope but that's pretty ugly.~

What's the best way?

Anyway, it's a solvable problem for sure.

[gavinking]

I'm not sure if we should make Mutiny.Session available for injection. Perhaps Uni<Mutiny.Session> is enough?

I like having both :)

[gavinking]

~OK, scratch all of what I wrote above. I don't see a good way to:~

~1. allow access to repositories from callbacks, and ~ ~2. also allow them to be injected into "wider" scopes (e.g. @ApplicationScoped beans). ~

~Those requirements seem to be fundamentally at odds.~

~But (1) looks necessary, and so we would need to drop (2).~

~But then if we're dropping (2), it appears that a viable "solution" here is actually quite simple: just make everything @Dependent-scoped. That's easy for us at least. But we should find a way to warn the user if they try to inject a repository or a session into something @SessionScoped or@ApplicationScoped.~

~So my code from above would change to:~

    @Produces //@Dependent 
    public Uni<Mutiny.StatelessSession> uniOfSession() {
        if (session == null) {
            throw new IllegalStateException("no session");
        }
        return Uni.createFrom().item(session);
    }
    @Produces //@Dependent
    public Mutiny.StatelessSession session() {
        if (session == null) {
            throw new IllegalStateException("no session");
        }
        return session;
    }

~And Hibernate Processor would stop generating the@RequestScoped annotation on repository implementations.~

~Am I missing something again?~

[gavinking]

I don't see a good way to:

• allow access to repositories from callbacks, and
• also allow them to be injected into "wider" scopes (e.g. @ApplicationScoped beans).

Those requirements seem to be fundamentally at odds.

Note that nothing about this problem is specific to session management. It's a problem with using a CDI normal-scoped bean from a reactive stream. I guess I had sorta assumed that this was something that worked in Quarkus. Do we properly document somewhere that this is dangerous in general?

UPDATE: Actually the very fact that there's nothing specific to session management here was the clue to unlock the realization that there's no actual problem, and my solution works fine modulo a very small bug.

[gavinking]

Wait wait wait. What if the code were this:

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

Isn't it the case that @RequestScoped stuff is stored in the Vert.x context, and will be available during execution of the whole stream?

If that's the case, the only problem in my code above was that I was unsetting the session reference too early.

[gavinking]

I just tested it with this code:

    @Inject Library library;

    @GET
    @Path("/books")
    public Uni<List<Book>> allBooks() {
        return Uni.createFrom().voidItem()
                .chain(v-> Uni.createFrom().nullItem())
                .chain(v -> library.byIsbn("xyz"))
                .onFailure().recoverWithNull()
                .chain(v -> library.allBooks())
                .invoke(list -> list.forEach(out::println));
    }

It appears to work correctly.

For good measure, I tried adding another level of indirection by injecting the repository bean into an intermediating bean, and calling it this way:

    @GET
    @Path("/books")
    public Uni<List<Book>> allBooks() {
        return Uni.createFrom().voidItem()
                .chain(v-> Uni.createFrom().nullItem())
                .chain(v -> bean.getAllBooks())
                .invoke(list -> list.forEach(out::println));
    }

Again it appears to work as expected.

So actually I don't think there's a real problem with laziness here.

Quarkus can already handle such "lazy" reactive access to @RequestScoped beans.

[gavinking]

So the best version I've come up with so far now looks like this:

import jakarta.enterprise.context.RequestScoped;
import org.hibernate.reactive.mutiny.Mutiny;
import org.hibernate.reactive.mutiny.delegation.MutinyStatelessSessionDelegator;

@RequestScoped
public class RequestScopedSession extends MutinyStatelessSessionDelegator {

    private Mutiny.StatelessSession session;
    private int reentrancy = 0;

    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++;
        }
    }

    @Override
    public Mutiny.StatelessSession delegate() {
        if (session == null) {
            throw new IllegalStateException("no session");
        }
        return session;
    }
}

import io.smallrye.mutiny.Uni;
import jakarta.inject.Inject;
import jakarta.interceptor.AroundInvoke;
import jakarta.interceptor.Interceptor;
import jakarta.interceptor.InvocationContext;
import org.hibernate.reactive.mutiny.Mutiny;

@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 {
                    Uni<?> result = (Uni<?>) invocationContext.proceed();
                    return result.eventually(() -> requestScopedSession.setSession(null));
                } catch (Exception e) {
                    throw new RuntimeException(e);
                }
            });
        }
        else {
            return invocationContext.proceed();
        }
    }
}

Note that it should be possible to eliminate the ugly-ass reentrance counter by exposing the current session on Mutiny.SessionFactory.

UPDATE: I simplified the code further.

[gavinking]

And finally, after the addition of getCurrentSession() to SessionFactory, I can simplify the code to:

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

@RequestScoped
public class RequestScopedStatelessSession extends MutinyStatelessSessionDelegator {

    private Mutiny.StatelessSession session;

    public void setSession(Mutiny.StatelessSession session) {
        this.session = session;
    }

    @Override
    public Mutiny.StatelessSession delegate() {
        if (session == null) {
            throw new IllegalStateException("no session");
        }
        return session;
    }
}

That's very simple and clean.

[mkouba]

Wait wait wait. What if the code were this:

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

Yes, we use Uni#eventually() in the current Panache integration... However, I think that I was wrong with "invoking injected Mutiny.Session lazily in the pipeline" in the sense that the producer method is only called when a contextual instance is created; i.e. requestScopedSession.setSession(null) does not have any effect and the contextual instance remains in the request context. In other words, any bean could inject the Mutiny.Session after a @WithSession method is invoked but the session might be closed/invalid.

[mkouba]

I'm not sure if we should make Mutiny.Session available for injection. Perhaps Uni<Mutiny.Session> is enough?

In any case, we should probably move SessionOperations, @WithSession, @WithTransaction to Quarkus-HR and untie it from HR/Panache, no?

Yes, that sounds reasonable. But it will be a breaking change :-(

[yrodiere]

In any case, we should probably move SessionOperations, @WithSession, @WithTransaction to Quarkus-HR and untie it from HR/Panache, no?

FYI, one thing we discussed lately around Hibernate Reactive was to make @Transactional work for Hibernate Reactive. I.e. having transaction-scoped sessions there, too -- and, critically, transaction-scoped connections, too, though I suppose that addresses only edge cases if the session is transaction-scoped anyway.

Maybe we could do that, instead of moving @WithTransaction?

cc @DavideD @tsegismont you probably have more to add here?

EDIT: And no, the intent is not to integrate with the transaction manager.

[FroMage]

That would be even better, especially if we can do away with @WithSession too :)

So big +1 from me.

[yrodiere]

That would be even better, especially if we can do away with @WithSession too :)

Well I'd argue @WithSession should die, because transactions are good, so @WithTransaction/@Transactional are everything a sane application should need (and others can rely on sessionFactory.withSession()), but I suppose we can at least support request-scoped, read-only, transaction-less sessions, like we do for Hibernate ORM.