Update Hibernate ORM, Hibernate Reactive #16923

Sanne · 2021-04-30T09:40:48Z

Fixes #16860
Fixes #16450
Fixes #16463
Fixes #16712

quarkus-bot · 2021-04-30T12:52:47Z

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building `c3dd446`

Status	Name	Step	Test failures	Logs	Raw logs
✖	JVM Tests - JDK 11	`Build`	Test failures	Logs	Raw logs
✖	JVM Tests - JDK 15	`Build`	Test failures	Logs	Raw logs
✖	Maven Tests - JDK 11	`Build`	Test failures	Logs	Raw logs
✖	Native Tests - Data5	`Build`	Test failures	Logs	Raw logs

Full information is available in the Build summary check run.

Test Failures

⚙️ JVM Tests - JDK 11 #

📦 integration-tests/hibernate-reactive-panache

✖ io.quarkus.it.panache.reactive.PanacheFunctionalityTest.testCompositeKey line 118 - More details - Source on GitHub

✖ io.quarkus.it.panache.reactive.PanacheFunctionalityTest.testPanacheFunctionality line 42 - More details - Source on GitHub

📦 integration-tests/hibernate-reactive-postgresql

✖ io.quarkus.it.hibernate.reactive.postgresql.HibernateReactiveTest.reactivePersist line 46 - More details - Source on GitHub

✖ io.quarkus.it.hibernate.reactive.postgresql.HibernateReactiveTest.reactiveRemoveTransientEntity line 54 - More details - Source on GitHub

✖ io.quarkus.it.hibernate.reactive.postgresql.HibernateReactiveTest.reactiveFindMutiny line 38 - More details - Source on GitHub

⚙️ JVM Tests - JDK 15 #

📦 integration-tests/hibernate-reactive-panache

✖ io.quarkus.it.panache.reactive.PanacheFunctionalityTest.testPanacheFunctionality line 45 - More details - Source on GitHub

📦 integration-tests/hibernate-reactive-postgresql

✖ io.quarkus.it.hibernate.reactive.postgresql.HibernateReactiveTest.reactivePersist line 46 - More details - Source on GitHub

✖ io.quarkus.it.hibernate.reactive.postgresql.HibernateReactiveTest.reactiveRemoveTransientEntity line 54 - More details - Source on GitHub

✖ io.quarkus.it.hibernate.reactive.postgresql.HibernateReactiveTest.reactiveFindMutiny line 38 - More details - Source on GitHub

⚙️ Maven Tests - JDK 11 #

📦 integration-tests/maven

✖ io.quarkus.maven.it.DevMojoIT.testThatNewResourcesAreServed line 841 - More details - Source on GitHub

⚙️ Native Tests - Data5 #

📦 integration-tests/hibernate-reactive-postgresql

✖ io.quarkus.it.hibernate.reactive.postgresql.HibernateReactiveAlternativeInGraalIT.reactiveRemoveManagedEntity - More details - Source on GitHub

✖ io.quarkus.it.hibernate.reactive.postgresql.HibernateReactiveAlternativeInGraalIT.reactiveUpdate - More details - Source on GitHub

✖ io.quarkus.it.hibernate.reactive.postgresql.HibernateReactiveAlternativeInGraalIT.reactiveRemoveTransientEntity - More details - Source on GitHub

yrodiere · 2021-04-30T12:56:06Z

@Sanne @DavideD @gavinking Plenty of failing tests for Hibernate Reactive after the upgrade... I see timeouts in particular. Could this be related to the session.close() changes?

Sanne · 2021-04-30T13:14:55Z

I'm sorry, I don't know. I guess it could relate, but things were not being closed correctly even before the patch so while it's certainly a suspect, it doesn't feel too likely either

DavideD · 2021-04-30T13:16:06Z

Weird, wasn't it tested before?

@yrodiere Possible, but it would surprise me a bit. We didn't have a reactive close in the previous version of Hibernate Reactive (even if the connection underneath was reactive) so connections were already closed the wrong way even before the update.

DavideD · 2021-04-30T13:17:29Z

Could it be related to the upgrade to ORM 5.5?
Other changes are the upgrade to Mutiny 0.16 and a bug fix to one-to-one associations

Sanne · 2021-04-30T13:45:28Z

Could it be related to the upgrade to ORM 5.5?

No we didn't upgrade to 5.5 yet

DavideD · 2021-04-30T13:50:41Z

The last suspicious thing I can think of is this blocking call: hibernate/hibernate-reactive@f0d59b2

What's the error anyway?

yrodiere · 2021-04-30T14:00:11Z

What's the error anyway?

I don't know more than what appears on the report; apparently the integration test times out. So I think something hangs forever in Reactive. May be a problem in tests, though they used to pass and they didn't change...

See https://github.com/quarkusio/quarkus/runs/2475746313

Sanne · 2021-04-30T17:17:23Z

Ok I think I have a more concrete idea. With the Mutiny API wrappers, an async method without any subscription is never invoked - this is different than a Future, in which case the lack of a subscription will defer the operation but it will still be run eventually.

So the previous version of Hibernate Reactive's close() implementation was:

    public void close() {
		delegate.close();
    }

Which was wrong as Vert.x 4 made the "close" asynchronous, but at least it would be triggered eventually.

But now that in the Mutiny version it was patched as:

	public Uni<Void> close() {
		return uni( delegate::reactiveClose );
	}

The problem is amplified in the Quarkus code as the CDI dispose method is implemented as:

quarkus/extensions/hibernate-reactive/runtime/src/main/java/io/quarkus/hibernate/reactive/runtime/ReactiveSessionProducer.java

Lines 35 to 45 in 09d03ab

    
           public void disposeStageSession(@Disposes Stage.Session reactiveSession) { 
        
               if (reactiveSession != null) { 
        
                   reactiveSession.close(); 
        
               } 
        
           } 
        
           public void disposeMutinySession(@Disposes Mutiny.Session reactiveSession) { 
        
               if (reactiveSession != null) { 
        
                   reactiveSession.close(); 
        
               } 
        
           }

N.B. the lack of subcriptions (not sure how to fix this in a CDI friendly way && without blocking)

So now the end effect is that since it's a Mutiny based wrapper and there is actually no subscription, it's never triggered and we run out of connections.

… 1.0.0.CR4

Sanne · 2021-05-04T18:02:55Z

Pushed something for CI to test - I'm having trouble testing with DB2 locally

quarkus-bot · 2021-05-04T21:24:04Z

Failing Jobs - Building `485d1f3`

Status	Name	Step	Test failures	Logs	Raw logs
✖	JVM Tests - JDK 11	`Build`	Test failures	Logs	Raw logs
✖	JVM Tests - JDK 11 Windows	`Build`	⚠️ Check →	Logs	Raw logs
✖	JVM Tests - JDK 15	`Build`	Test failures	Logs	Raw logs
✖	Maven Tests - JDK 11	`Build`	Test failures	Logs	Raw logs
✖	MicroProfile TCKs Tests	`Verify`	Test failures	Logs	Raw logs

Full information is available in the Build summary check run.

Test Failures

⚙️ JVM Tests - JDK 11 #

📦 core/test-extension/deployment

✖ io.quarkus.extest.ConfiguredBeanTest.testConfigDefaultValuesSourceOrdinal line 346 - More details - Source on GitHub

⚙️ JVM Tests - JDK 15 #

📦 core/test-extension/deployment

✖ io.quarkus.extest.ConfiguredBeanTest.testConfigDefaultValuesSourceOrdinal line 346 - More details - Source on GitHub

⚙️ Maven Tests - JDK 11 #

📦 integration-tests/maven

✖ io.quarkus.maven.it.DevMojoIT.testThatNewResourcesAreServed line 841 - More details - Source on GitHub

⚙️ MicroProfile TCKs Tests #

📦 tcks/microprofile-config

✖ org.eclipse.microprofile.config.tck.ConfigPropertiesTest.testConfigPropertiesDefaultOnBean line 172 - More details - Source on GitHub

✖ org.eclipse.microprofile.config.tck.ConfigPropertiesTest.testConfigPropertiesNoPrefixOnBean line 149 - More details - Source on GitHub

✖ org.eclipse.microprofile.config.tck.ConfigPropertiesTest.testConfigPropertiesNoPrefixOnBeanThenSupplyPrefix line 156 - More details - Source on GitHub

✖ org.eclipse.microprofile.config.tck.ConfigPropertiesTest.testConfigPropertiesPlainInjection line 106 - More details - Source on GitHub

✖ org.eclipse.microprofile.config.tck.ConfigPropertiesTest.testConfigPropertiesWithPrefix line 115 - More details - Source on GitHub

✖ org.eclipse.microprofile.config.tck.ConfigPropertiesTest.testConfigPropertiesWithoutPrefix line 132 - More details - Source on GitHub

✖ org.eclipse.microprofile.config.tck.ConfigPropertiesTest.testNoConfigPropertiesAnnotationInjection line 165 - More details - Source on GitHub

Sanne · 2021-05-05T04:58:44Z

Soo still some failing tests but they seem unrelated to my changes.

WDYT @yrodiere @gsmet ?
We need this, lots of progress is being stalled, so while I don't like CI status I'm inclined to merge

yrodiere

LGTM apart from a comment that seems misleading. In any case, +1 to merge as the failures seem completely unrelated.

yrodiere · 2021-05-05T06:29:45Z

...ive/runtime/src/main/java/io/quarkus/hibernate/reactive/runtime/ReactiveSessionProducer.java

@@ -34,13 +34,24 @@

    public void disposeStageSession(@Disposes Stage.Session reactiveSession) {
        if (reactiveSession != null) {
-            reactiveSession.close();
+            //We're ignoring the returned CompletionStage!


I agree with the change but I think the comment should be:

Suggested change

//We're ignoring the returned CompletionStage!

//We must not ignore the returned CompletionStage!

I'm actually referring to the fact that the disposeStageSession method itself (the one we're changing here) is returning void - while it should chain the event back to a reactive chain. That's sadly not possible with CDI at the moment.

So "must not ignore" would IMO be confusing as we are actually ignoring it here?

yrodiere · 2021-05-05T06:29:51Z

...ive/runtime/src/main/java/io/quarkus/hibernate/reactive/runtime/ReactiveSessionProducer.java

        }
    }

    public void disposeMutinySession(@Disposes Mutiny.Session reactiveSession) {
        if (reactiveSession != null) {
-            reactiveSession.close();
+            //We're ignoring the returned CompletionStage!


I agree with the change but I think the comment should be:

Suggested change

//We're ignoring the returned CompletionStage!

//We must not ignore the returned CompletionStage!

Sanne · 2021-05-05T07:38:22Z

LGTM apart from a comment that seems misleading. In any case, +1 to merge as the failures seem completely unrelated.

right - and I've checked now that the other failures are indeed reproducible w/o my patch. Stuart also seems to have just fixed one of them in #16986

I'll merge - thanks!

Sanne · 2021-05-05T09:10:09Z

Sadly, I've decided to remove the "backport" label:

Let's try to backport these fixes only if there's strong need - the problem is that while I'm comfortable with the changes in ORM, there is a high coupling requiring the HR upgrade as well - which now requires Vertx4 in this release.

Backporting would need an alternative stream of HR artifacts. cc/ @gavinking @DavideD

gavinking · 2021-05-05T09:34:24Z

Off the top of my head I don't think it would be that hard to do a dedicated HR release against the pre-vert.x 4 version. But i'm having a hard time gauging the priority. How important is this?

Sanne · 2021-05-05T09:40:31Z

@gavinking I think we can live just fine without one for now, I only mentioned it so you're aware that we might need one in the future.

We should also learn about such constraints, clearly I got the release sequence puzzle wrong this time - sorry

gavinking · 2021-05-05T09:42:32Z

OK. Let me know if you change your mind. I really don't think it would be a big deal to do a release.

DavideD · 2021-05-05T11:28:13Z

I didn't check the whole consversation, but releasing different versions of hibernate reactive is pretty straight forward. For different versions of vertx (3 9 and 4.0) we need to maintain different branches though.

…

On Wed, 5 May 2021, 10:42 Gavin King, ***@***.***> wrote: OK. Let me know if you change your mind. I really don't think it would be a big deal to do a release. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16923 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEIQ5PTULDJ335BFUM3IHLTMEHJXANCNFSM43326W3A> .

Sanne added area/persistence OBSOLETE, DO NOT USE triage/backport? area/hibernate-orm Hibernate ORM area/hibernate-reactive Hibernate Reactive labels Apr 30, 2021

quarkus-bot bot added the area/dependencies Pull requests that update a dependency file label Apr 30, 2021

Sanne requested a review from yrodiere April 30, 2021 09:41

yrodiere approved these changes Apr 30, 2021

View reviewed changes

yrodiere added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Apr 30, 2021

Sanne added 4 commits May 4, 2021 18:47

Upgrade to Hibernate ORM 5.4.31.Final and matching Hibernate Reactive…

dfec0f8

… 1.0.0.CR4

Regression test for issue quarkusio#16860 resolved by HHH-14575

27dc85b

Do not ignore return values for reactiveSession#close

77d7936

Adapting the ThreadLocalPool wrapper to close resources asynchronously

485d1f3

Sanne force-pushed the BasicProxies branch from c3dd446 to 485d1f3 Compare May 4, 2021 18:00

yrodiere self-requested a review May 5, 2021 06:28

yrodiere approved these changes May 5, 2021

View reviewed changes

Sanne merged commit 1d994fe into quarkusio:main May 5, 2021

quarkus-bot bot added this to the 2.0 - main milestone May 5, 2021

Sanne deleted the BasicProxies branch May 5, 2021 07:39

quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label May 5, 2021

Sanne removed the triage/backport? label May 5, 2021

markusdlugi mentioned this pull request Jun 30, 2021

Hibernate Reactive: ReactiveSessionProducer blocks event loop thread #18268

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Hibernate ORM, Hibernate Reactive #16923

Update Hibernate ORM, Hibernate Reactive #16923

Sanne commented Apr 30, 2021 •

edited

Loading

quarkus-bot bot commented Apr 30, 2021 •

edited

Loading

Failing Jobs - Building `c3dd446`

Test Failures

⚙️ JVM Tests - JDK 11 #

📦 integration-tests/hibernate-reactive-panache

📦 integration-tests/hibernate-reactive-postgresql

⚙️ JVM Tests - JDK 15 #

📦 integration-tests/hibernate-reactive-panache

📦 integration-tests/hibernate-reactive-postgresql

⚙️ Maven Tests - JDK 11 #

📦 integration-tests/maven

⚙️ Native Tests - Data5 #

📦 integration-tests/hibernate-reactive-postgresql

yrodiere commented Apr 30, 2021

Sanne commented Apr 30, 2021

DavideD commented Apr 30, 2021 •

edited

Loading

DavideD commented Apr 30, 2021 •

edited

Loading

Sanne commented Apr 30, 2021

DavideD commented Apr 30, 2021

yrodiere commented Apr 30, 2021

Sanne commented Apr 30, 2021 •

edited

Loading

Sanne commented May 4, 2021

quarkus-bot bot commented May 4, 2021

Sanne commented May 5, 2021

yrodiere left a comment

yrodiere May 5, 2021

Sanne May 5, 2021

yrodiere May 5, 2021

Sanne commented May 5, 2021

Sanne commented May 5, 2021

gavinking commented May 5, 2021

Sanne commented May 5, 2021

gavinking commented May 5, 2021

DavideD commented May 5, 2021 via email

	//We're ignoring the returned CompletionStage!
	//We must not ignore the returned CompletionStage!

Update Hibernate ORM, Hibernate Reactive #16923

Update Hibernate ORM, Hibernate Reactive #16923

Conversation

Sanne commented Apr 30, 2021 • edited Loading

quarkus-bot bot commented Apr 30, 2021 • edited Loading

Failing Jobs - Building c3dd446

Test Failures

⚙️ JVM Tests - JDK 11 #

📦 integration-tests/hibernate-reactive-panache

📦 integration-tests/hibernate-reactive-postgresql

⚙️ JVM Tests - JDK 15 #

📦 integration-tests/hibernate-reactive-panache

📦 integration-tests/hibernate-reactive-postgresql

⚙️ Maven Tests - JDK 11 #

📦 integration-tests/maven

⚙️ Native Tests - Data5 #

📦 integration-tests/hibernate-reactive-postgresql

yrodiere commented Apr 30, 2021

Sanne commented Apr 30, 2021

DavideD commented Apr 30, 2021 • edited Loading

DavideD commented Apr 30, 2021 • edited Loading

Sanne commented Apr 30, 2021

DavideD commented Apr 30, 2021

yrodiere commented Apr 30, 2021

Sanne commented Apr 30, 2021 • edited Loading

Sanne commented May 4, 2021

quarkus-bot bot commented May 4, 2021

Failing Jobs - Building 485d1f3

Test Failures

⚙️ JVM Tests - JDK 11 #

📦 core/test-extension/deployment

⚙️ JVM Tests - JDK 15 #

📦 core/test-extension/deployment

⚙️ Maven Tests - JDK 11 #

📦 integration-tests/maven

⚙️ MicroProfile TCKs Tests #

📦 tcks/microprofile-config

Sanne commented May 5, 2021

yrodiere left a comment

Choose a reason for hiding this comment

yrodiere May 5, 2021

Choose a reason for hiding this comment

Sanne May 5, 2021

Choose a reason for hiding this comment

yrodiere May 5, 2021

Choose a reason for hiding this comment

Sanne commented May 5, 2021

Sanne commented May 5, 2021

gavinking commented May 5, 2021

Sanne commented May 5, 2021

gavinking commented May 5, 2021

DavideD commented May 5, 2021 via email

Sanne commented Apr 30, 2021 •

edited

Loading

quarkus-bot bot commented Apr 30, 2021 •

edited

Loading

Failing Jobs - Building `c3dd446`

DavideD commented Apr 30, 2021 •

edited

Loading

DavideD commented Apr 30, 2021 •

edited

Loading

Sanne commented Apr 30, 2021 •

edited

Loading

Failing Jobs - Building `485d1f3`