[4.2] Prevent context switching during id generation #2793

DavideD · 2025-11-18T13:29:45Z

Follows #2690 and it's a refactoring of @tsegismont suggestion.

I changed the warning about the swithching context into assertions
I had to change where we read the context when a new connection is created. The problem is that the session factory can be used in a worker thread at start up, and therefore the context could be null. This is the line that saves the context
ProxyConnection synchronize the creation of a connection using reflection. I don't know if this can be a problem with Quarkus.

For now we use it only when the connection is used

Now the test will throw an exception if context and connection aren't handled correctly.

This commit: * Stop connection leaks in BlockingIdentifierGenerator * Assert that the connection is used in the expected context

yrodiere

Thanks @DavideD @tsegismont!

yrodiere · 2025-11-19T11:55:17Z

...rnate-reactive-core/src/main/java/org/hibernate/reactive/common/InternalStateAssertions.java

 		}
 	}

+	public static void assertCurrentContextMatches(Object object, ContextInternal expectedContext, Exception creationTrace) {


With this new assertion, do we still need assertCurrentThreadMatches? If not, fixing #2736 would be trivial...

I don't know :)

Seems like it, but I feel that this PR is complicated enough. I will look into it as soon as we merge this.

hibernate-reactive-core/src/main/java/org/hibernate/reactive/pool/impl/SqlClientPool.java

yrodiere · 2025-11-19T12:09:24Z

hibernate-reactive-core/src/main/java/org/hibernate/reactive/pool/impl/SqlClientPool.java

+		static {
+			try {
+				MethodHandles.Lookup lookup = MethodHandles.lookup();
+				OPENED_HANDLE = lookup.findVarHandle( ProxyConnection.class, "opened", boolean.class );


This is new to me, but it looks like a common pattern in CompletableFuture. I guess the idea is to get access to compareAndSet without the overhead of AtomicBoolean. Whether this overhead is significant in this specific case is quite questionable if you ask me, but I assume Thomas knows better than I do.

In any case, back to your question @DavideD: this kind of "reflection" in a static block should generally be fine in Quarkus even with native compilation, because it's executed during static init, before native compilation. If I turn out to be wrong, we'll improvise :)

The idea was that, since the application will create a lot of these objects during its lifetime, it would be better to store a simple boolean field in ProxyConnection instead of a complete object (own headers + field).

@franz1981 would you recommend against it?

In any case, back to your question @DavideD: this kind of "reflection" in a static block should generally be fine in Quarkus even with native compilation, because it's executed during static init, before native compilation. If I turn out to be wrong, we'll improvise :)

Thanks, that's all I needed in writing :D

yrodiere · 2025-11-19T12:23:09Z

hibernate-reactive-core/src/main/java/org/hibernate/reactive/pool/impl/SqlClientPool.java

+		public ProxyConnection withBatchSize(int batchSize) {
+			connectionFuture.thenApply( reactiveConnection -> reactiveConnection.withBatchSize( batchSize ) );
 			return this;
 		}


This seems wrong?

The method is supposed to return a batched connection, and from what I can see we rely on the return value:

hibernate-reactive/hibernate-reactive-core/src/main/java/org/hibernate/reactive/session/impl/ReactiveSessionImpl.java

Lines 1753 to 1757 in 066917f

@Override

public void setBatchSize(Integer batchSize) {

setJdbcBatchSize( batchSize );

reactiveConnection = reactiveConnection.withBatchSize( batchSize );

}

Returning this will not work because we don't update the delegate, so if the caller uses this, they'll still be using a non-batching connection.

I'll assume the contract is that this can be ignored after this method is called, and only the return value will be used by the caller. Which seems consistent with what I'm seeing in BatchedConnection#withBatchSize(), where we brutally change the batch size of an existing instance.

If that assumption is correct, I'd suggest replacing this code with something like:

Suggested change

public ProxyConnection withBatchSize(int batchSize) {

connectionFuture.thenApply( reactiveConnection -> reactiveConnection.withBatchSize( batchSize ) );

return this;

}

public ProxyConnection withBatchSize(int batchSize) {

ReactiveConnection reactiveConnection = connectionFuture.getNow( null );

if (reactiveConnection != null) {

// Common case (hopefully): connection was already opened,

// we can let callers use the delegate going forward and forget about the proxy.

return reactiveConnection.withBatchSize(batchSize);

}

else {

// Uncommon case (hopefully): connection wasn't opened yet,

// we will ask callers to use a new proxy that wraps the connection.

return new ProxyConnection( () -> connectionSupplier.get().withBatchSize( batchSize ) );

}

}

NOTE: If you add something to mark the proxy as closed (see my comment on close), it would be a good idea mark the current proxy as closed in the else block here, just in case someone (incorrectly) goes against the assumption that I stated above.

yrodiere · 2025-11-19T12:26:17Z

hibernate-reactive-core/src/main/java/org/hibernate/reactive/pool/impl/SqlClientPool.java

+			return opened
+					? connectionFuture.getNow( null ).close()
 					: voidFuture();


Shouldn't we remember that close was called in the proxy, so that if another method is called on the proxy, we don't mistakenly create the delegate?

I'm talking about the second case in particular:

Create connection proxy

Call close(): nothing happens

Callexecute(...): delegate gets created! Instead of erroring out.

yrodiere · 2025-11-19T12:41:19Z

...-core/src/test/java/org/hibernate/reactive/MultithreadedInsertionWithLazyConnectionTest.java

+			// We are not using transactions on purpose here, because this approach will cause a context switch
+			// and an assertion error if things aren't handled correctly. See Hibernate Reactive issue #2768:
+			// https://github.com/hibernate/hibernate-reactive/issues/2768


Would it make sense to duplicate the test, to also test the "transaction" case?

Yeah, it makes sense.

yrodiere · 2025-11-19T12:52:02Z

...-reactive-core/src/main/java/org/hibernate/reactive/id/impl/BlockingIdentifierGenerator.java

+			nextHiValue( connectionSupplier )
+					.whenComplete( (newlyGeneratedHi, throwable) -> {
+						if ( throwable != null ) {
+							result.completeExceptionally( throwable );
+						}
+						else {
+							//We ignore the state argument as we actually use the field directly
+							//for convenience, but they are the same object.
+							executor.submit( stateIgnored -> {
+								result.complete( next( newlyGeneratedHi ) );
+								return null;
+							} );
+						}
+					} );


Are we sure nextHiValue will never throw an exception? I mean the method, not the future it returns.

Because if it does throw an exception, the whenComplete call here will be skipped and the operation will hang forever from the user's point of view.

DavideD added 3 commits November 18, 2025 09:24

[hibernate#2768] Add exception for the execution on the wrong context

a4c9b67

For now we use it only when the connection is used

[hibernate#2768] Update existing test

a7dc0a1

Now the test will throw an exception if context and connection aren't handled correctly.

[hibernate#2768] Avoid context switching on id generation

9dce31a

This commit: * Stop connection leaks in BlockingIdentifierGenerator * Assert that the connection is used in the expected context

DavideD requested a review from yrodiere November 19, 2025 11:50

DavideD added this to the 4.2.0.Beta1 milestone Nov 19, 2025

yrodiere reviewed Nov 19, 2025

View reviewed changes

DavideD force-pushed the 2768-Connection-context branch from e4fdfd4 to 9dce31a Compare November 19, 2025 17:42

	@Override
	public void setBatchSize(Integer batchSize) {
	setJdbcBatchSize( batchSize );
	reactiveConnection = reactiveConnection.withBatchSize( batchSize );
	}

-		public ProxyConnection withBatchSize(int batchSize) {
-			connectionFuture.thenApply( reactiveConnection -> reactiveConnection.withBatchSize( batchSize ) );
-			return this;
-		}
+		public ProxyConnection withBatchSize(int batchSize) {
+		  ReactiveConnection reactiveConnection = connectionFuture.getNow( null );
+		  if (reactiveConnection != null) {
+			// Common case (hopefully): connection was already opened,
+			// we can let callers use the delegate going forward and forget about the proxy.
+		    return reactiveConnection.withBatchSize(batchSize);
+		  }
+		  else {
+			// Uncommon case (hopefully): connection wasn't opened yet,
+			// we will ask callers to use a new proxy that wraps the connection.
+			return new ProxyConnection( () -> connectionSupplier.get().withBatchSize( batchSize ) );
+		  }
+		}

Uh oh!

[4.2] Prevent context switching during id generation #2793

Are you sure you want to change the base?

[4.2] Prevent context switching during id generation #2793

Uh oh!

Conversation

DavideD commented Nov 18, 2025

Uh oh!

yrodiere left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants