Live-Reload not working with Hibernate Reactive

[Sanne]

Describe the bug

Live-reload doesn't work when using Hibernate Reactive.

To Reproduce

Using the hibernate-reactive-quickstart, start it in dev-mode

1. hit endpoint localhost:8080/fruits
2. change the named query declared on Fruit from "SELECT f FROM Fruit f ORDER BY f.name" to "SELECT f FROM Fruit f ORDER BY f.id"
3. hit endpoint localhost:8080/fruits

This throws an exception, apparently we're not dropping the original classloader fully, and the entity class doesn't match the expected type.

Quarkus version or git rev

2.0.0.Final

[quarkus-bot]

/cc @DavideD, @gavinking

[gavinking]

weird, i'm so sure I tested this!

[DavideD]

To clarify, live reload doesn't work only if one changes something in the entity. It works if one changes something in the other classes

…

On Thu, Jul 1, 2021 at 10:31 AM Gavin King ***@***.***> wrote: weird, i'm so sure I tested this! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18299 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEIQ5I27Z422W73EPWO4ELTVQYYPANCNFSM47UDUCOA> .

[Sanne]

@gavinking to be clear, yes it worked in the past. It's a recent regression; apparently we don't have tests for this.

[gavinking]

OK

[Sanne]

More details on duplicate issue: #18301

[DavideD]

I think you linked to this issue instead of the duplicate one

[Sanne]

thanks, fixed.

[Sanne]

@stuartwdouglas would you be able to look at this? If not I can try myself, but not before next week.

[stuartwdouglas]

So this will work if you do -Dvertx.disableTCCL=true (although you get a heap of warnings in the logs). The fundamental issue here is that vert.x hangs onto the original TCCL, and we can't update it. With Vert.x 3 we had a hack that allowed us to modify the TCCL, however this is no longer working as the Vert.x Context explicitly sets the TCCL to the old CL for every operation.

[markusdlugi]

Hi @stuartwdouglas, thanks a lot for the insights! I can confirm that setting -Dvertx.disableTCCL=true will fix the issue, named queries will work again. Do you know whether this can have any other negative side effects, or is this a safe workaround (other than printing out a lot of warnings)?

Besides that, any idea how we could fix this properly? I still find it interesting that this happens if you use sessionFactory.withSession() but not if you inject Mutiny.Session. Is it due to the former making use of the Vert.x Context for storing the session? Do we need to ensure that any sessions are removed from the context upon reload?

As a side note, I noticed that the other problem I described in #18301 that an injected SecurityContext stops working actually happens regardless of hot reload. So once you try to access a SecurityContext within a withSession() block you get the mentioned error, no matter if the application was reloaded or not. Therefore, that issue is also not fixed by setting -Dvertx.disableTCCL=true. So that might be another issue unrelated to the TCCL issue that causes named queries to fail.

[Sanne]

@markusdlugi it should be a safe workaround, but clearly this hasn't been tested much.

@stuartwdouglas interesting, thanks. FIY with the Quarkus benchmarks on Techempower, we had -Dvertx.disableTCCL=true for Quarkus 1.x on Julien's suggestion as it helps a little with performance; we then took this flag out when migrating to previews of Quarkus 2.x because of the warning (obviously the warning was not helping performance).

So I guess ideally we'd want to be able to force vert.x to not capture the classloader? Would be nice to remove the overhead (w/o warnings) and without having the need to set the flag. cc/ @vietj

[stuartwdouglas]

We have two different requirements here:

• Prod mode: Set the classloader at thread creation, use the same one the whole time (i.e. just use the inherited one), so basically vert.x can do nothing
• Dev mode: Change the 'captured' CL on each reload.

I think this will need some vert.x changes to make this work in an ideal way, previously this was all done with some nasty hacks, and the first application CL would always leak as vert.x would never release it.

[markusdlugi]

How can we proceed here? Do we need to open an issue for vert.x?

We do have the workaround with -Dvertx.disableTCCL=true but it's rather ugly due to the many warnings so it kinda ruins the dev mode user experience. We haven't experienced any negative side effects with that workaround as of yet, but can't really tell if there actually are none.

[stuartwdouglas]

I guess we could just filter out the warnings.

I think with this flag set this is basically the same as the Vert.x 3 behavior, so it should be ok. We still have this hack:

quarkus/extensions/vertx-core/runtime/src/main/java/io/quarkus/vertx/core/runtime/VertxCoreRecorder.java

Line 129 in fadd877

private void tryCleanTccl(Vertx devModeVertx) {

which will update the TCCL on restart.

We probably should look at fixing this in Vert.x though.

[Sanne]

@stuartwdouglas I don't understand these hacks, in particular I wonder how we can get away with sunddenly setting a new CL on the context; wouldn't it be more appropariate to shut down the current context in a clean way and then start the new ones with the new context?

[stuartwdouglas]

Then you lose the live reload workflow, as the first request after a change will fail as the server is shut down

[Sanne]

But also I wonder about the real purpose of these:

https://github.com/eclipse-vertx/vert.x/blob/ccbe23c30a6cf0f93e221b447a3b49897eddb175/src/main/java/io/vertx/core/impl/ContextImpl.java#L79-L81

if it's not critical for vertx that we do stick to these rules, perhaps the fix is just to remove those lines in Vertx ? But I'd love to hear @vietj 's opinion about that, as I'm sure the warning has some history.

[Sanne]

Then you lose the live reload workflow, as the first request after a change will fail as the server is shut down

Is there no way to gracefully spin up new workers while terminating the stale ones?

[Sanne]

Github's autocompletion doesn't seem to like me pinging vietj so I suspect that's not working: sent an email on the list.

[stuartwdouglas]

The only thing 'stale' about them is that they have the old TCCL. Trying to attach a new event loop to an existing connection is not really possible without ltd of much more complex and invasive changes that likely would not be accepted into Netty. Stuart

…

On Wed, 21 Jul 2021, 7:48 pm Sanne Grinovero, ***@***.***> wrote: Then you lose the live reload workflow, as the first request after a change will fail as the server is shut down Is there no way to gracefully spin up new workers while terminating the stale ones? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18299 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACQG62PM2SWD3ZOGDWXXRLTY2JV5ANCNFSM47UDUCOA> .

[Sanne]

@stuartwdouglas today I'm looking at leaks again, and it seems even setting the vertx.disableTCCL doesn't prevent various instances of io.quarkus.bootstrap.classloading.QuarkusClassLoader being leaked, a different one being held by each VertxThread. I suppose this suggests that tryCleanTccl isn't working anymore.

[Sanne]

The problem with tryCleanTccl seems to be that each of the many "reset tasks" it is scheduling for execution via executeBlocking is actually going to be executed on a single, sole thread as a new context is created for the first task, and then associated with the current thread. I'll try to see what alternatives vertx offers.

[stuartwdouglas]

Yea, tryCleanTccl was introduced for Vert.x 3, changes for Vert.x 4 seem to have broken it.