Consider forcing Uni return types on all Session opening methods

[Sanne]

I've got burned once again by chaining the wrong style of Session opening.

This is what I had as a general helper:

 @Inject
Mutiny.SessionFactory sf;

public <T> Uni<T> inSession(Function<Mutiny.Session, Uni<T>> work) {
    final Mutiny.Session session = sf.openSession();
    return sf.withSession( work ).eventually( session::close );
}

(This is very similar to the withSession helper offered on MutinySessionImpl, but I was needing to intentionally side-step the use of the org.hibernate.reactive.context.Context - details not relevant now)

When I'm passing as work argument a trivial work which does a single operation on the session, this works as expected.

But if I use such an helper method with a slightly less trivial work, such as needing to load more than one entity, the second load operation is likely going to hit the infamous error we already know quite well:

Caused by: java.lang.IllegalStateException: Session/EntityManager is closed
	at org.hibernate.internal.AbstractSharedSessionContract.checkOpen(AbstractSharedSessionContract.java:393)
	at org.hibernate.engine.spi.SharedSessionContractImplementor.checkOpen(SharedSessionContractImplementor.java:148)
	at org.hibernate.reactive.session.impl.ReactiveSessionImpl.checkOpen(ReactiveSessionImpl.java:1556)
	at org.hibernate.internal.AbstractSharedSessionContract.checkOpenOrWaitingForAutoClose(AbstractSharedSessionContract.java:399)

In this case, I am sure that this was all run on the same vert.x context (and the same thread); with @DavideD 's help we figured out the problem is that the Mutiny.Session instance opened this way is not fit to be used more than once, because the composition of multiple operations on it can't guarantee that the session proxy is being initialized once and atomically.

I would propose to remove the Session openSession(); method and its siblings dealing with tenant-id and stateless sessions, to be replaced with methods which return the Uni<Session>, Uni<StatelessSession>, etc...

Impact:

• it would seem the Hibernate Reactive code would get simpler
• most Quarkus integration code would also benefit from it
• Panache Reactive would also be simpler, as it is more common to require a Uni<Session> - the only exception being the boolean isPersistent(Object entity) method which Panache Reactive exposes, but we can either deal with that by using the context, or update the signature to also become a reactive method.

[Sanne]

cc/ @gavinking I'd be keen to hear you opinion on this one.

[gavinking]

Well if I understand correctly, your helper method is simply wrong.

We document fairly clearly, I think, that sessions obtained with openSession() aren't "managed" and don't propagate.

[Sanne]

I'm not expecting propagation, I'm passing the Session instance explicitly to the functions.

My only error was in expecting that I could actually use such a Session for composite operations, such as loading multiple entities with individual statements

[gavinking]

Then I just don't understand your code. You open one session, then you do the work in a whole different session, then you close the first session.

I don't understand the logic there.

[gavinking]

We document fairly clearly

Well, TBH, perhaps it's not that well documented...

[DavideD]

I think @Sanne's example was supposed to be:

public <T> Uni<T> inSession(Function<Mutiny.Session, Uni<T>> work) {
    final Mutiny.Session session = sf.openSession();
    return work.apply(session).eventually( session::close );
}

[gavinking]

Aaah OK that makes more sense.

[gavinking]

the problem is that the Mutiny.Session instance opened this way is not fit to be used more than once, because the composition of multiple operations on it can't guarantee that the session proxy is being initialized once and atomically.

Well if that's the case then that would be a bug.

But to be really honest I'm simply having some trouble believing you Sanne. Every time someone has complained about the proxy, it has turned out to be misuse of the session. I've stared at that code really hard in the past, and I've always managed to convince myself it's right.

[Sanne]

oops yes sorry wrong code example. I've been going back and forth with various experiments.

But to be really honest I'm simply having some trouble believing you Sanne. Every time someone has complained about the proxy, it has turned out to be misuse of the session. I've stared at that code really hard in the past, and I've always managed to convince myself it's right.

I totally understand the feeling, we've had many such cases and most often there was some mistake in the user code, or tricky threading. In this case though while I was 99% sure I was making some mistake, I then asked Davide to help me out as I couldn't figure it out - but this time around my conclusion was that really returning an Uni<Session> would really prevent many similar problems and also simplify the code quite a bit.

Tangentially, but we also concluded that my code was OK in this case :) And that worries me a apparently this idea of the connection proxy is making things really tricky. And as I mentioned, all integration code I know of would actually prefer dealing with an Uni.

I guess we should make a test for Hibernate Reactive for you to see, that would simplify the discussion - I can't do that today though.

[gavinking]

I mean, this code works perfectly:

public <T> Uni<T> inSession(Function<Mutiny.Session, Uni<T>> work) {
	final Mutiny.Session session = getMutinySessionFactory().openSession();
	return work.apply(session).eventually( session::close );
}

@Test
public void method(TestContext context) {
	test( context, inSession(
			(s) -> s.persist(new GuineaPig( 5, "Aloi" ))
					.call( () -> s.persist(new GuineaPig( 6, "Pig" )))
					.chain( ()-> s.createQuery("from GuineaPig").getResultList() )
	).invoke( list -> context.assertEquals(2, list.size() )) );
}

and so I can't imagine how it could break unless you're doing something dodgy with concurrency.

[gavinking]

this time around my conclusion was that really returning an Uni<Session> would really prevent many similar problems and also simplify the code quite a bit.

Well if you recall when I made that change last time, everyone rebelled against it and talked me out of it. So I'm kinda wary of going back there.

[Sanne]

So, I failed miserably in reproducing the issue in the context of an Hibernate Reactive integration test, but it fails consistently in the benchmark (even without load, it fails on a single call).

The cause seems to be this code in which I need to load a bag of entities by having the set of Ids:

this fails (in the benchmark only) when using the openSession():

public Uni<List<World>> findManaged(Mutiny.Session s, Set<Integer> ids) {
    List<Uni<? extends World>> l = new ArrayList<>(ids.size());
    for (Integer id : ids) {
        l.add(s.find(World.class, id));
    }
    return Uni.join().all(l).andFailFast();
}

The problem seems to be that Uni.join().all(l) will trigger the load of all find operations w/o necessarily waiting for the first one to be completed before initiating the operation of the second: it merely guarantees the composition is completed when each individual operation is completed.

The documentation rightfully points out something related [http://hibernate.org/reactive/documentation/1.0/reference/html_single/#_obtaining_a_reactive_session]

It’s very important to understand that most operations of this interface are non-blocking, and execution of SQL against the database is never performed synchronously. Persistence operations that belong to a single unit of work must be chained by composition within a single reactive stream.

So changing the operation to explicitly chain all ops will work fine:

   public Uni<List<World>> findManaged(Mutiny.Session s, Set<Integer> ids) {
    final List<World> worlds = new ArrayList<>();
    Uni<Void> loopRoot = Uni.createFrom().voidItem();
    for (Integer id : ids) {
        loopRoot = loopRoot.chain( () -> s.find(World.class, id).invoke( word -> worlds.add( word ) ).replaceWithVoid() );
    }
    return loopRoot.map( v -> worlds );
}

So this raises multiple questions:

1. Is this limitation really ok? I understand the concerns about using the Session "concurrently" but this isn't really concurrency, and it feels like an important element from Reactive programming that we shouldn't be flagging as illegal
2. Both methods above will work fine if you use the inSession() method rather than the openSession because inSession doesn't require a lazily initialized ProxyConnection.
3. I really need to figure out why this is straight forward to reproduce in Quarkus but not in the HR codebase.

[gavinking]

Well, right, at least as far as I've always understood it, join() isn't threadsafe and this is something that's prohibited.

[DavideD]

Well if you recall when I made that change last time, everyone rebelled against it and talked me out of it. So I'm kinda wary of going back there.

Initially it sounded nice to have openSession return something that's not reactive, but time and time again these type of methods are causing us problems (see when session.close was non-reactive for example). And I'm starting to get suspicious every time I see one.

So I wouldn't mind to reconsider our position now that we have more experience.

EDIT: I've removed some already mentioned points

[gavinking]

Well no, I don't think that's right. The connection isn't the only volatile state held by the session; far from it.

[gavinking]

Oh, looks like I responded to text you deleted.

[gavinking]

I think you guys need to consider that changing the return type of openSession() won't do anything to eliminate this class of bugs. It just makes it much more likely that the user won't notice them, or rather that they'll manifest in a way that's much harder to diagnose. (Concurrent access to session state is going to result in problems that are much less transparent than this one.)

[Sanne]

Well, right, at least as far as I've always understood it, join() isn't threadsafe and this is something that's prohibited.

I suppose you're using the term "threadsafe" loosely in this context, in which case I agree with you.. but let me be precise in case we're misunderstanding each other: this is all running on the same, single thread, so the canonical definition of "threadsafe" doesn't really apply.

We might need to clarify some things in the doc.

I think you guys need to consider that changing the return type of openSession() won't do anything to eliminate this class of bugs.

But that's the point I'm raising. IFF there a real problem with using Uni.join().all(l), should we work harder to detect the issue consistently?

As things stand, we only fail hard if you're using the current openSession() implementation as it just so happens that the ProxyConnection will help spotting the problem by failing fast. We don't really raise any complaint if the sessions are opened in a different way.

On the other hand, IFF we state that using Uni.join().all(l) is "just fine" provided you use the session via the SessionFactory#withSession helper , then it would also work totally fine if we change the openSession contract to return a Uni, getting rid of the ProxyConnection.

There's a third alternative: I have a local POC which adapts the ProxyConnection implementation to deal with it correctly - I'm just unsure about us wanting to go in that direction: if you say this is fundamentally wrong we shouldn't support it.

[gavinking]

I suppose you're using the term "threadsafe" loosely

Right.

But that's the point I'm raising. IFF there a real problem with using Uni.join().all(l), should we work harder to detect the issue consistently?

Well, how? We already do the best we can, by checking at the Vert.x context. Apparently that's not good enough to detect the problem in this case, but I don't see what else we could do if Mutiny doesn't expose some sort way to identify the "micro-context".

[Sanne]

Ok fair enough.

So the conclusion is that using Uni.join().all(l) (or similar) is not something we want to support? I'm just not 100% clear on why we shouldn't but I'm happy to take your word on it for now and close this.

[gavinking]

Well it's not that I don't want to it's that I have no idea how to.

[Sanne]

this would make it work:

• Amend ProxyConnection to deal with multiple requests being scheduled #953

I'm just not sure if we want to :) And what other issues I'm possibly missing.

[gavinking]

I don't see how that would fix the generic problem of concurrent access to volatile session state. During every single (nonblocking) database request, the session is in an inconsistent state.

I don't see how you can fix that without having some sort of "transactional" management of the session's internal data structures.

[Sanne]

Ok yes I think I understand what you mean, although I'm not sure in practice which scenarios would be problematic; for sure this would work just fine for loading a sequence of trivial entities with no relations among each other (as I was doing here).

So while we agree that multi-threaded access on the same Session is a big no-no, I'm not sure how far we can expect a "reactive programmer" to not issue subsequent requests for data operations without honouring a strict chain of operations - doesn't that defeat the purpose of the model?

As far as I had understood, one of the significant benefits of reactive programming is that it becomes unnecessary to explicitly load N items in bulk, as one can just start requesting each of the N items one by one and effectively have them returned at a similar total dealy.

IMO this is a big limitation which we should look at better; specifically I'd love to see specific tests leading to inconsistent Session state, so to possibly wrap our head around it in a more concrete way.

[DavideD]

It just makes it much more likely that the user won't notice them, or rather that they'll manifest in a way that's much harder to diagnose. (Concurrent access to session state is going to result in problems that are much less transparent than this one.)

I'm not sure how I feel about this. If a user uses withSession, they won't notice this issue anyway. I think I'd prefer a consistent behaviour. It also seems that in some simpler scenarios this might be fine when using withSession but not when using openSession.

Making the signature of the method reactive has also the benefit of simplifying the code and I suspect it will save us some headaches in the future.

[gavinking]

It also seems that in some simpler scenarios this might be fine

Nope, it's never fine. It's not fine in regular Hibernate, and it's not fine here.

(There's absolutely nothing in what I've said here that's specific to Reactive, FTR.)

Making the signature of the method reactive has also the benefit of simplifying the code and I suspect it will save us some headaches in the future.

I think the much more relevant question here is: for someone who does want to manage their own reactive Sessions, for example, in some sort of interceptor or something, what is the most convenient API for them.

I'm not quite clear on what the answer to that is.

[DavideD]

You mean, for example, what's more convenient between @Inject Session session and @Inject Uni<Session> sessionUni?

[Sanne]

I'm not quite clear on what the answer to that is.

I'm not entirely sure either, but I have a draft of adapting Quarkus (and Panache Reactive) to deal with @Inject Uni<Session> instead of @Inject Session and it did simplify some code, allowing me to delete several "helpers"

[gavinking]

You mean, for example, what's more convenient between @Inject Session session and @Inject Uni<Session> sessionUni?

I mean what's more convenient for someone implementing that sort of injection, for example.