Panache and Hibernate Reactive use different Vert.x local contexts #47441
Conversation
quarkus-bot
bot
added
area/core
area/dependencies
|
@DavideD it seems there are unrelated changes in this PR, correct? |
|
|
|
Nevermind, I rebased it and now this is the commit with the important changes: 3093299 |
...me/src/main/java/io/quarkus/hibernate/reactive/panache/common/runtime/SessionOperations.java
Outdated
Show resolved
Hide resolved
In preparation to the upgrade to Vertx. 5, Hibernate Reactive doesn't use anymore the deprecated API to access the local context. This commits updates the way Panache access the context so that the two extensions can comunicate. Fix quarkusio#47314 Relats to the Hibernate Reactive issues: * hibernate/hibernate-reactive#2176 * hibernate/hibernate-reactive#2174 * hibernate/hibernate-reactive#2207
|
@tsegismont I moved the method to access the map in ContextualDataStorage, and updated this PR. Would this work? Do you think I also need to udpate the following code in VertxContextSafety? I don't think so but I'm not sure. |
We could avoid to retrieve the Vert.x context twice, is that what you were thinking about? |
No, I was just wondering if you see something that might need to be updated because of the new API. |
Well at some point (Vert.x 5 upgrade at the latest) Quarkus will have to move away from storing data in local context map. |
| @@ -50,8 +52,10 @@ public Key<Session> get() { | |||
| }); | |||
|
|
|||
| // This key is used to indicate that a reactive session should be opened lazily (when needed) in the current vertx context | |||
| private static final String SESSION_ON_DEMAND_KEY = "hibernate.reactive.panache.sessionOnDemand"; | |||
| private static final String SESSION_ON_DEMAND_OPENED_KEY = "hibernate.reactive.panache.sessionOnDemandOpened"; | |||
| private static final Key<Boolean> SESSION_ON_DEMAND_KEY = new BaseKey<>(Boolean.class, | |||
There was a problem hiding this comment.
Do not use the "key"-based API for the context local for the time being (and probably for a long time). Quarkus won't support it — we are not even sure if we will support it in Quarkus 4.
There was a problem hiding this comment.
In Hibernate Reactive we've been using it for a while... should I change it there too?
Should I only use String? What's the issue with using an interface as key?
There was a problem hiding this comment.
Or by key-based you mean I shouldn't use a Map? What should I use instead?
There was a problem hiding this comment.
Using Context local keys is not inherently wrong, in fact it's the method promoted by Vert.x (and works just fine for Hibernate Reactive when used out of Quarkus). What @cescoffier is saying is that a lot of Quarkus extensions expect to manipulate contextual in a common map, and he thinks it might be preferable to keep it this way.
That said, I'm not sure a lot of Quarkus extensions need to know the internals of contextual data storage for Hibernate Reactive, so perhaps we can stick to the changes introduced in hibernate/hibernate-reactive#2177
At this point, I'm waiting on @cescoffier to clarify the requirements for contextual data storage so that we can decide how to move forward, upstream (Vert.x, Hibernate Reactive) and in Quarkus core.
There was a problem hiding this comment.
Quarkus needs to be able to list keys, copy values and manipulate the map - that's how we make sure what is propagated and what is not. Using Key objects makes this very inconvenient and would need to duplicate the whole process. So, switching to that new API means that some feature will not work with Hibernate Reactive.
There was a problem hiding this comment.
Ok, so the only solution right now is to stick to the deprecated API and revert the changes in Hibernate Reactive, right? Or, instead of using a Key interface, use a String.
@tsegismont so… is context-storage going away? What is it replaced with? |
The common contextual data map is not longer accessible from the public API in Vert.x 5. @vietj and I have started to talk about this with @cescoffier and we need to clarify the requirements. |
|
OK, thanks. Lettuce know ;) |
|
If I understand correctly, there's no real issue with this approach except that we will need to change it as soon as something new has been introduced. I can release Hibernate Reactive with this changes for now, right? |
If there are extensions that depend on how Hibernate Reactive stores contextual data, I would wait for @cescoffier to clarify which ones before releasing. Reading the comments from @FroMage , it sounds like Panache is one of them |
|
I think Panache is the only one that needs some coordination with Hibernate Reactive. But OK, let's wait a bit |
|
As long as Panache and HR use the same means of storing the session we're fine. We can change HR to match Panache, or change Panache to match HR, it doesn't matter :) |
|
Apparently there's also a difference in how Vert.x and Quarkus create duplicated contexts (Quarkus supports nested context). That could impact the expectations regarding contextual data. So I'd wait for @cescoffier to chime in |
|
I don't want to disturb your discussion, but we're still waiting for this as fix for #47314, which is currently present in Quarkus 3.21.3 and 3.21.4 (and soon also 3.22). This bug has the potential to cause serious issues in productive applications, and it's likely that this is also affecting other HR Panache users - we were only lucky to spot it due to our good test coverage. So while I understand and appreciate the desire to find the best solution, please keep this in mind. Maybe it might be worth it to separate any major refactoring work into its own issue? Especially since the actual requirements still seem to be unclear as far as I understood your discussion. |
|
Good point. Let's get a fix for this issue, and a test ASAP, and we'll see about further improvements later. |
|
I'm going to revert the changes I've introduced in Hibernate Reactive and continue to use the deprecated API. We can replace them when there is a suitable solution. |
Possible fix for #47314
Draft because it needs these changes in Hibernate Reactive (next release).
@tsegismont, or @vietj, Do these changes make sense?
Thanks