Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hibernate Reactive thread model fixes #17728

Merged
merged 12 commits into from
Jul 1, 2021

Conversation

FroMage
Copy link
Member

@FroMage FroMage commented Jun 7, 2021

  • Changed Mutiny.Session Panache.getSession to Uni<Mutiny.Session> Panache.getSession
  • Changed boolean PanacheEntity.isPersistent to Uni<Boolean> PanacheEntity.isPersistent
  • Added @ReactiveTransaction to forward to Mutiny.Session.withTransaction: method must return a Uni or be invoked from a non-VertxThread (in which case it blocks)
  • Make sure we create Mutiny.Session objects from the right threads
  • Still rely on the request-scoped Mutiny.Session and not the Vert.x context one used by Mutiny.Session.withSession
  • Added Panache.currentTransaction which forwards to Mutiny.Session.currentTransaction
  • Added a guide
  • Fixed a mocking issue wrt interfaces

@Sanne @gavinking this should resolve the issues we've been having, right?

@gavinking
Copy link

Why didn't you implement out Context interface?

@FroMage
Copy link
Member Author

FroMage commented Jun 7, 2021

I guess I could, but why would it be better than this?

@gavinking
Copy link

Well ... try it?

From a superficial look at your code, I would say it probably would be simpler, but I can't be sure without trying.

@FroMage
Copy link
Member Author

FroMage commented Jun 7, 2021

Also, do we already have a configurable timeout for HR in Quarkus?

@gavinking
Copy link

A what?

@FroMage
Copy link
Member Author

FroMage commented Jun 7, 2021

A timeout for waiting for a DB response.

@Sanne
Copy link
Member

Sanne commented Jun 7, 2021

A timeout for waiting for a DB response.

no we don't have timeouts

@gavinking
Copy link

I would not think that HR would be the right layer to do something like that.

@FroMage
Copy link
Member Author

FroMage commented Jun 23, 2021

OK, this time I've added the missing @ReactiveTestTransaction and rebased on main. Let's see what CI says about this.

@geoand
Copy link
Contributor

geoand commented Jun 23, 2021

@FroMage is there anything you want me to check WRT the PR?

@FroMage
Copy link
Member Author

FroMage commented Jun 23, 2021

Yeah, changes in the test-framework from a69fb12 Do you want me to extract them to their own commit?

I've added some functions, and made sure that the finally block that terminated the request context was moved to the end of the reactive call, and introduced something truely horrible to make sure that I could get interceptors to contribute to the asserter to contribute to the reactive pipeline.

@geoand
Copy link
Contributor

geoand commented Jun 23, 2021

Yeah, changes in the test-framework from a69fb12 Do you want me to extract them to their own commit?

No need

I've added some functions,

👍🏼

made sure that the finally block that terminated the request context was moved to the end of the reactive call

very good idea, this is much better than what I had!

introduced something truely horrible to make sure that I could get interceptors to contribute to the asserter to contribute to the reactive pipeline.

Not a thing of beauty for sure, but let's not get hung up on this detail as this PR should significantly improve the user experience

@geoand
Copy link
Contributor

geoand commented Jun 24, 2021

CI is all green 🎉

@FroMage
Copy link
Member Author

FroMage commented Jun 24, 2021

I think we should just merge this ASAP, because the API differences are breaking, so it needs to be in 2.0. Even if the way we store the session/transaction changes in the future, the API should be the same. Besides, it fixes real issues, so it's just better than what we have ATM.

@geoand
Copy link
Contributor

geoand commented Jun 24, 2021

You have my vote, but I'll leave it up to the folks tagged as reviewers

@Sanne
Copy link
Member

Sanne commented Jun 24, 2021

because the API differences are breaking, so it needs to be in 2.0.

hum I see two problems with this statement :/

  • for 2.0 this is too late
  • we've being saying that we do actually not see the "2.0" major as a particular reason to break backwards compability, we can break it any time (and should always try to avoid doing so). Which is good news in the context?

Changed boolean PanacheEntity.isPersistent to Uni PanacheEntity.isPersistent

Could you elaborate on why?

@FroMage
Copy link
Member Author

FroMage commented Jun 24, 2021

for 2.0 this is too late

Ah.

Could you elaborate on why?

Yes, since obtaining the session is now async, all operations that require it are async.

@Sanne
Copy link
Member

Sanne commented Jun 24, 2021

Could you elaborate on why?

Yes, since obtaining the session is now async, all operations that require it are async.

yea I can see that you implement this by getting a Session first, but if you don't have a session we shouldn't need to open one just for this check, right?

Which implies you don't need to change the signature.

I mean, conceptually this method doesn't need to run any async IO operation - we should be able to return the boolean by inspecting the current state.

@FroMage FroMage force-pushed the hr-panache-thread-fix branch from e2e9de5 to 2551acd Compare June 25, 2021 12:50
@FroMage
Copy link
Member Author

FroMage commented Jun 25, 2021

OK done.

@FroMage
Copy link
Member Author

FroMage commented Jun 25, 2021

Ah, lemme check the docs.

@FroMage
Copy link
Member Author

FroMage commented Jun 25, 2021

Hah, the docs were wrong, so they're now correct by accident :) Brilliant!

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 25, 2021

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

Failing Jobs - Building 2551acd

Status Name Step Test failures Logs Raw logs
JVM Tests - JDK 11 Build ⚠️ Check → Logs Raw logs
✔️ JVM Tests - JDK 16

@FroMage
Copy link
Member Author

FroMage commented Jun 28, 2021

Failure is not related: 2021-06-25T16:15:43.7464154Z Caused by: io.fabric8.maven.docker.access.hc.http.HttpRequestException: {"message":"Head https://registry-1.docker.io/v2/neo4j/neo4j-experimental/manifests/4.0.0-rc01: unknown: unable to scan account service mfa settings row from query: context deadline exceeded"} (Internal Server Error: 500)

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 28, 2021

Failing Jobs - Building 2551acd

Status Name Step Test failures Logs Raw logs
Maven Tests - JDK 11 Build Test failures Logs Raw logs
Native Tests - Misc4 Build ⚠️ Check → Logs Raw logs

Full information is available in the Build summary check run.

Test Failures

⚙️ Maven Tests - JDK 11 #

📦 integration-tests/maven

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

@geoand
Copy link
Contributor

geoand commented Jun 29, 2021

So is this good to go?

@Sanne
Copy link
Member

Sanne commented Jun 30, 2021

it probably is - I had asked @DavideD to play a bit more with it but I guess we can also choose to play with it after it's integrated.

@geoand
Copy link
Contributor

geoand commented Jun 30, 2021

Up to you obviously.

Just want to mention that with 2.1 still a ways out, playing with it from main should not jeopardize the deliverable

@FroMage
Copy link
Member Author

FroMage commented Jun 30, 2021

I don't know what I have to do to get a review 🤷

@holomekc
Copy link
Contributor

holomekc commented Jul 1, 2021

Hi a short question. Did you try to use hot reload while using withSession and withTransaction. We implemented an interceptor similar to ReactiveTransactionalInterceptor and in our case a hot reload destroy something, when using one of the named methods.

It fails with:

java.lang.IllegalArgumentException: Type specified for TypedQuery [....entity.Agent] is incompatible with query return type [class ....entity.Agent]
	at org.hibernate.internal.AbstractSharedSessionContract.resultClassChecking(AbstractSharedSessionContract.java:878) ~[hibernate-core-5.5.3.Final.jar:5.5.3.Final]
	at org.hibernate.reactive.session.impl.ReactiveSessionImpl.createReactiveQuery(ReactiveSessionImpl.java:510) ~[hibernate-reactive-core-1.0.0.CR7.jar:1.0.0.CR7]
	at org.hibernate.reactive.session.impl.ReactiveSessionImpl.buildReactiveQueryFromName(ReactiveSessionImpl.java:487) ~[hibernate-reactive-core-1.0.0.CR7.jar:1.0.0.CR7]
	at org.hibernate.reactive.session.impl.ReactiveSessionImpl.createReactiveNamedQuery(ReactiveSessionImpl.java:472) ~[hibernate-reactive-core-1.0.0.CR7.jar:1.0.0.CR7]
	at org.hibernate.reactive.mutiny.impl.MutinySessionImpl.createNamedQuery(MutinySessionImpl.java:251) ~[hibernate-reactive-core-1.0.0.CR7.jar:1.0.0.CR7]
...
  at io.quarkus.arc.impl.AroundInvokeInvocationContext.proceed(AroundInvokeInvocationContext.java:54) ~[arc-2.0.0.Final.jar:2.0.0.Final]
  at io.quarkus.arc.runtime.devconsole.InvocationInterceptor.proceed(InvocationInterceptor.java:62) ~[quarkus-arc-2.0.0.Final.jar:?]
  at io.quarkus.arc.runtime.devconsole.InvocationInterceptor.monitor(InvocationInterceptor.java:49) ~[quarkus-arc-2.0.0.Final.jar:?]
  at io.quarkus.arc.runtime.devconsole.InvocationInterceptor_Bean.intercept(InvocationInterceptor_Bean.zig:521) ~[quarkus:/:?]
  at io.quarkus.arc.impl.InterceptorInvocation.invoke(InterceptorInvocation.java:41) ~[arc-2.0.0.Final.jar:2.0.0.Final]
  at io.quarkus.arc.impl.AroundInvokeInvocationContext.perform(AroundInvokeInvocationContext.java:41) ~[arc-2.0.0.Final.jar:2.0.0.Final]
  at io.quarkus.arc.impl.InvocationContexts.performAroundInvoke(InvocationContexts.java:32) ~[arc-2.0.0.Final.jar:2.0.0.Final]

in general context information seems to be broken after hot reload in this case. E.g. SecurityContext crashes with NullpointerException while accessing context and some metrics which we registered at startup are also not available afterwards.

We switched to withSession and withTransaction because of this issue:
#18268 (comment)

and here the suggestion to switch to mentioned methods:
#14845

We did not create an issue for this yet. But it would be great if you could check if you face similar issues in your pr.

@geoand
Copy link
Contributor

geoand commented Jul 1, 2021

There is a very good chance the testing stuff introduced in this PR will start having conflicts soon. For that reason and because this stuff seems to be important for the user experience (not to mention there is plenty of time to iron out whatever small issues it might have), a review would be much appreciated

@Sanne
Copy link
Member

Sanne commented Jul 1, 2021

@geoand & @FroMage sorry I was off the last 2 days - catching up soon. Tangentially, but this happened other times as well.. and I'm sorry but I'm often dragged into urgent matters related to Hibernate, we should try to grow more people able to review Panache PRs as I can't be the one you need to wait for.

@holomekc I've noticed the same and can reproduce it without Panache as well. I've just opened #18299

@geoand
Copy link
Contributor

geoand commented Jul 1, 2021

@geoand & @FroMage sorry I was off the last 2 days - catching up soon. Tangentially, but this happened other times as well.. and I'm sorry but I'm often dragged into urgent matters related to Hibernate, we should try to grow more people able to review Panache PRs as I can't be the one you need to wait for.

Yeah, that is totally understood :)

Copy link
Member

@Sanne Sanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left some minor notes, but would prefer integrating sooner and improve later on those, if you think they are worthwhile.

}
----

TIP: If you don't want to bother defining getters/setters for your entities, you can make them extend `PanacheEntityBase` and
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know :)
But what is the goal? Can't we allow users to not use getters and setters at all?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can, but by making sure everything goes via property accessors, we guarantee that they can turn the model classes from a field to a property in a backward compatible way. Not sure it's that useful in practice.

- `set? <update-query>` will expand to `update from EntityName set <update-query>`

NOTE: You can also write your queries in plain
link:https://docs.jboss.org/hibernate/orm/5.4/userguide/html_single/Hibernate_User_Guide.html#hql[HQL]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no biggie, but we switched to ORM 5.5

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, probably doesn't change much.

PanacheQuery<PersonName> query = Person.find("status", Status.Alive).project(PersonName.class);
----

1. The `@RegisterForReflection` annotation instructs Quarkus to keep the class and its members during the native compilation. More details about the `@RegisterForReflection` annotation can be found on the xref:writing-native-applications-tips.adoc#registerForReflection[native application tips] page.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

humm I really don't like that people have to "manually" use the annotation. It was created as a "we don't have time to fix things properly today" band-aid, but it seems we're suggesting its usage more and more.

Could you possibly look at making this unnecessary? We don't require it for any other Hibernate project.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, this is only for cases where we do a projection, and since the projection class is not annotated with anything, we can't piggy-back on that annotation to detect it. We sould scan for calls to the .project(Class) method, now that jandex indexes use-sites, but, not sure it's worth it, this is pretty rare, non?


1. The `@RegisterForReflection` annotation instructs Quarkus to keep the class and its members during the native compilation. More details about the `@RegisterForReflection` annotation can be found on the xref:writing-native-applications-tips.adoc#registerForReflection[native application tips] page.
2. We use public fields here, but you can use private fields and getters/setters if you prefer.
3. This constructor will be used by Hibernate, it must be the only constructor in your class and have all the class attributes as parameters.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure it must be "the only constructor" ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure. https://docs.jboss.org/hibernate/orm/5.5/userguide/html_single/Hibernate_User_Guide.html#hql-select-clause says "The projection class must be fully qualified in the entity query, and it must define a matching constructor." Perhaps we can clarify this.

reluctantly deal with, such as:

- Duplicating ID logic: most entities need an ID, most people don't care how it's set, because it's not really
relevant to your model.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realy don't agree, but ... ok well your choice.


Hibernate Reactive with Panache relies on compile-time bytecode enhancements to your entities.

It attempts to identity archives with Panache entities (and consumers of Panache entities)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/identity/identify/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup

if (Vertx.currentContext() == context) {
runnable.run();
} else {
// this needs to be sync
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't you already check that this is not running on a vertx thread?

if (io.vertx.core.Context.isOnVertxThread()) {
return Uni.createFrom().item(Arc.container().instance(Mutiny.Session.class).get());
} else {
// FIXME: we may need context propagation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate on what could be missing here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well context propagation, using MP-CP, but I'd rather delay that until someone reports it.

}
}

private static boolean isInRequestContext(Class<?> klass) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me sad, is there no efficient way to check if a bean already exists in the currenct context, without needing to go through all these hoops?

I was hoping there was something like Map.contains( key )

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(it's of course an orthogonal issue we can address later)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW I was exploring some alternatives here but got in deep trouble with bytecode ... that's what the zulip thread is about.

@Sanne Sanne merged commit 8057395 into quarkusio:main Jul 1, 2021
@Sanne
Copy link
Member

Sanne commented Jul 1, 2021

Many thanks @FroMage ! merged it

@quarkus-bot quarkus-bot bot added this to the 2.1 - main milestone Jul 1, 2021
@edeandrea
Copy link
Contributor

Question @FroMage - do you know if the @ReactiveTransaction stuff is going to make it into a 2.0.x release (2.0.1 hopefully)?

@geoand
Copy link
Contributor

geoand commented Jul 2, 2021

It will almost certainly not, it's too big of a change

@edeandrea
Copy link
Contributor

Thanks @geoand

@geoand
Copy link
Contributor

geoand commented Jul 6, 2021

With this in, I assume we can start looking into supporting Kotlin Coroutines with Hibernate Reactive Panache (cc @evanchooly)?

@evanchooly
Copy link
Member

That's the plan, yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants