Guarantee serialization of calls to the session

[yrodiere]

A user can write this:

return sessionFactory.withSession(s -> {
   return Uni.combine().combine().all().unis(s.doSomething(), s.doSomethingElse()).asTuple();
});

It is incorrect, because it could result in interleaved uses of the session, messing up its state: doSomething() could set up some temporary state, do some IO so release control of the event loop, then doSomethingElse() would start executing, mess up the temporary state, and we're in a bad place.

This section of the docs explains a related problem -- not the same one, but the explanations are relevant.

Unfortunately -- on contrary to the problem mentioned in the docs -- this problem is not detected and will be very hard to debug.

More importantly... it's probably just as easy to fix as it is to detect. We could simply make sure that all operations in the session start running after the previous one finished.

E.g. this code in ReactiveSessionImpl:

	@Override
	public <T> CompletionStage<T> reactiveFind(
			Class<T> entityClass,
			Object id,
			LockOptions lockOptions,
			EntityGraph<T> fetchGraph) {
		checkOpen();
		return supplyStage( () -> {
			...
		} );

Could become:

	@Override
	public <T> CompletionStage<T> reactiveFind(
			Class<T> entityClass,
			Object id,
			LockOptions lockOptions,
			EntityGraph<T> fetchGraph) {
		checkOpen();
		this.previousOperation = this.previousOperation.thenCompose(ignored -> supplyStage( () -> {
			...
		} ) );
		return previousOperation;

(Or better yet, we'd integrate this with supplyStage, which seems to be doing something similar with a voidFuture)

previousOperation would just be a field in the session, initialized to a completed future.

We could do this for StatelessSession as well, though it will be harder to explain why state can be a problem with a "stateless" session...

[yrodiere]

Note the suggested solution could... perhaps... possibly... cause big trouble if some session operations call other session operations. I'm not sure, but it's something to be careful about.

[Sanne]

I think you're on a reasonable track, but bear in mind that if you compose operations in such a way you might end up executing a section of the chain multiple times - and still possibly in a different eventloop.

Another limitation would be that not all operations which affect the Session are initiated by the Session API: for such a trick to work consistently one would need to make sure all entry points have a similar safety net.

Overall, I believe a stricter check which throws errors might be a safer approach - the reason being that it guides the user into a better design, rather than them possibly relying on the safety mechanism, especially as the safety mechanism wouldn't be water tight.

Another factor of confusion from the combine().all().unis( approach is that you might end up with an A,B chain in some tests, and then execute B,A in a production environment :(
Your approach would make sure they don't happen in parallel but it's still confusing for people: possibly unintended order, and not running in "parallel" as they might be expecting.

[Sanne]

N.B. if you check the sources of ReactiveSessionImpl you'll also notice assertions for:

• InternalStateAssertions.assertUseOnEventLoop()
• never having switched the actual thread

I originally had even stronger checks, such as being on the same "virtual thread" (vert.x context) - but that caused issues when the Vert.x client had to re-balance the pool and switch. I'm now thinking perhaps we should keep the stricter checks, and allow the pool rebalance as the only case in which it's allowed.

[yrodiere]

you might end up executing a section of the chain multiple times

I think CompletableFuture has run-once guarantees?

still possibly in a different eventloop

That I can believe, I've been hit before. Special care is certainly needed. But I believe we only need to care about using the right executor, and I imagine the solution can be found all around the Reactive codebase. If we mandate (and check) that a session is only ever used from a single event loop (and I think we do), the solution is probably simple.

Overall, I believe a stricter check which throws errors might be a safer approach - the reason being that it guides the user into a better design, rather than them possibly relying on the safety mechanism, especially as the safety mechanism wouldn't be water tight.

Maybe as a complementary solution? I really don't think that writing Uni.combine().combine().all().unis(s.doSomething(), s.doSomethingElse()) is crazy, and throwing exceptions for simple things that we can handle is going to hurt UX a lot.

Another factor of confusion from the combine().all().unis( approach is that you might end up with an A,B chain in some tests, and then execute B,A in a production environment :(
Your approach would make sure they don't happen in parallel but it's still confusing for people: possibly unintended order, and not running in "parallel" as they might be expecting.

True, but then that's a limitation of Reactive programming in general, not just Hibernate Reactive. Thinking about it, I seem to remember some programming languages don't guarantee execution order of expressions passed as arguments -- even for imperative programming!

[yrodiere]

Overall, I believe a stricter check which throws errors might be a safer approach - the reason being that it guides the user into a better design, rather than them possibly relying on the safety mechanism, especially as the safety mechanism wouldn't be water tight.

Maybe as a complementary solution? I really don't think that writing Uni.combine().combine().all().unis(s.doSomething(), s.doSomethingElse()) is crazy, and throwing exceptions for simple things that we can handle is going to hurt UX a lot.

Also I'm not sure we can have 100% effective checks... When you write Uni.combine().combine().unis(s.doSomething(), s.doSomethingElse()), IIRC neither doSomething or doSomethingElse will start executing until someone actually subscribes to the combined uni, and then it seems possible that doSomething and doSomethingElse would execute one after another in the best (worst?) case scenario?

[Sanne]

I really don't think that writing Uni.combine().combine().all().unis(s.doSomething(), s.doSomethingElse()) is crazy, and throwing exceptions for simple things that we can handle is going to hurt UX a lot.

It's a very good point honestly. Very well let's try this - I enumerated some possible complications, and like you say some are possibly unfounded, not with the goal to discourage you but to try making it robust.

Wondering if one could customize the Uni.combine() "combiner" strategy so to have it combine things in a safe way?

cc/ @jponge ?

[jponge]

Thanks @Sanne, but I'm afraid I don't have a good short-term answer.

This comes back every now and then, and the conclusion we had is that we can't easily and reliably detect the pipeline structure and emit library-custom warnings if such constructs are being made.

Joining concurrent I/O operation results is a good thing, but yes it doesn't play right with libraries that have thread-state assumptions.

Note that you will get the very same kind of issues with structured concurrency and virtual threads, because users will be tempted to make concurrent requests, you won't always have any control on which thread takes over after results have been joined, and the very same problem will be there.

[yrodiere]

Hey @jponge I've been thinking about this again, and I wonder how bad our use of Vert.x contexts is when using Uni.combine()...

In particular, if we do this:

Uni.combine().combine().all().unis(
    doSomething(),
    doSomethingElse()
);

Then, assuming:

1. Both doSomething() and doSomethingElse() mutate the Vert.x context using the same key (e.g. they both use Panache.withSession/SessionFactory#withSession)
2. both doSomething() and doSomethingElse() do IO, meaning their callbacks could be interleaved

... then, there is a chance the Vert.x context from doSomething() would leak into doSomethingElse(), correct?

And the only solution to completely avoid this would be for doSomething() and doSomethingElse() to create a duplicated context instead of mutating the context... correct?

[jponge]

Hi @yrodiere, can you please point me to what HR is doing on the threads / contexts side?

What I can tell you is that the last thread to complete an I/O and emit (either doSomething() or doSomethingElse()) will be the one carrying over the combination operation duty.

[yrodiere]

Hi @yrodiere, can you please point me to what HR is doing on the threads / contexts side?

It would be this, among others: https://github.com/quarkusio/quarkus/blob/26e8896599a898f6a2534fc4f74e5df089667152/extensions/panache/hibernate-reactive-panache-common/runtime/src/main/java/io/quarkus/hibernate/reactive/panache/common/runtime/SessionOperations.java#L149-L164

And I'm wondering if contexts could possibly leak from one "thread" to another in code like this:

Uni.combine().combine().all().unis(
    withSession( _ -> doSomething() ),
    withSession( _ -> doSomethingElse() )
);

... or like this:

Uni.combine().combine().all().unis(
    doSomething1().chain( () -> withSession( _ -> doSomething2() ) ),
    doSomethingElse1().chain( () -> withSession( _ -> doSomethingElse2() ) )
);

... assuming the various doSomething/doSomethingElse will read the context that was set by withSession, and assuming both operations perform multiple IO and involve multiple callbacks.

What I can tell you is that the last thread to complete an I/O and emit (either doSomething() or doSomethingElse()) will be the one carrying over the combination operation duty.

Understood... though I'm more worried about what's happening between the start of the first thread and the end of the last one 😬

[jponge]

I need to dig this at some point, or over a call with you one of these days.

I don't think the vertxContext gives you a duplicated context, but just the current context (which may be a duplicated one, but at this stage you don't have any guarantee). I'd re-use https://github.com/smallrye/smallrye-common/blob/main/vertx-context/src/main/java/io/smallrye/common/vertx/VertxContext.java

Now speaking of contexts, instead of tying your context to threads / duplicated contexts, note that Mutiny has a notion of context that's bound to the subscription (see https://smallrye.io/smallrye-mutiny/latest/guides/context-passing/). It is initiated by the downstream subscriber, and the rest of the pipeline operators can access it, including those in a combine or join construct.