Change how JPAConfig cleans up its resources

[marko-bekhta]

this is an alternative to #45451.

Adding a priority to shutdown tasks allows us to have a better understanding of how those will be executed. Having the task firing the shutdown event as the first one in that queue seemed reasonable; that way, those handling this event will not be in a situation where something gets shut down before the event handler can use it.

Then, for the ORM-specific part, we add a shutdown task when we create a JPA config, and the cleanup of PUs will only close those that actually correctly started.

I thought about removing the lastShutdownTasks queue and just handling that through priority, but then ... I didn't want to change too much 😄 so I've kept it for now.

• Closes: Exception on application shutdown with quarkus-hibernate-search-orm-coordination-outbox-polling #34547

[quarkus-bot]

/cc @gsmet (hibernate-orm,hibernate-search)

[github-actions]

🎊 PR Preview 1a01fee has been successfully built and deployed to https://quarkus-pr-main-46357-preview.surge.sh/version/main/guides/

• Images of blog posts older than 3 months are not available.
• Newsletters older than 3 months are not available.

[yrodiere]

Thanks.

IMO introducing priorities makes sense and we definitely should go in this direction.

But...

First, we'd need more careful definition/documentation :)

Second, and more importantly, I wonder if this isn't stopping short of what we really need. For example shouldn't we tie startup order to this? It kind of makes sense that components should be shut down in the opposite order they started in, especially for synthetic beans using .startup() (like datasources).

We could imagine something like:

1. Startup of components outside of CDI
2. Startup of Quarkus "core" CDI beans
3. Startup of Quarkus "extension" CDI beans, in particular:
   1. Startup of Datasources
   2. Startup of Hibernate
4. Startup of application beans
5. Shutdown of application beans
6. Shutdown of Quarkus "extension" CDI beans, in particular:
   1. Shutdown of Hibernate
   2. Shutdown of Datasources
7. Shutdown of Quarkus "core" CDI beans
8. Shutdown of components outside of CDI

... which looks a lot like Intereceptor.Priority, but probably shouldn't be based on it, because I think items 1 & 9 are extra.

What your PR covers is, I think, items 5/6. I'm fine with starting with that, but we should probably raise the issue to others, and perhaps make sure that the priorities we'll pick here will make sense when/if we extend your work to all startup/shutdown.

[yrodiere]

This is suspicious... integration tests should be like an application, so shouldn't need to rely on "internal" modules?

Did you try putting these tests in extensions/hibernate-search-orm-elasticsearch-outbox-polling/deployment instead?

[yrodiere]

This is apparently still relevant... maybe?

[marko-bekhta]

good point, thanks 😃
moved them around and it worked (at least locally) let's see what CI says 🤞🏻 🙂

[marko-bekhta]

ohh and there are more changes in main now 😁 let me rebase ....

[marko-bekhta]

First, we'd need more careful definition/documentation :)

1000000000000000000% 😄 I just wanted to push this out as an idea and have something to start the discussion around 😃.

Second, and more importantly ....

yeah, there are all sorts of flavours for these shutdown things, there are observers for a shutdown event, there are shutdown
tasks, and then also bean destroyers (if it's a CDI bean...) and maybe something else I'm unaware 😃.

so my thinking here was that on shutdown we:

• fire the shutdown event, so user-defined observers would get a chance to do their work while all the things are not yet closed
• shutdown extensions in a required order. e.g. we know that we want to shutdown ORM while CDI is still up, so we give it a higher priority and keep the CDI shutdown as a last one in this batch. Shutting down CDI will trigger calling all the bean destroyers (in a random order)
• and then shut down the last batch (things currently in lastShutdownTasks e.g. vertx etc..)

so I'd create a priority range for these groups and then use that.

[yrodiere]

so my thinking here was that on shutdown we:

* fire the shutdown event, so user-defined observers would get a chance to do their work while all the things are not yet closed

* shutdown extensions in a required order. e.g. we know that we want to shutdown ORM while CDI is still up, so we give it a higher priority and keep the CDI shutdown as a last one in this batch. Shutting down CDI will trigger calling all the bean  destroyers (in a random order)

* and then shut down the last batch (things currently in lastShutdownTasks e.g. vertx etc..)

so I'd create a priority range for these groups and then use that.

Fine by me, but like I said this needs to take into account future changes, because changing these constants later will be painful.

In particular:

1. Make sure the constants would still make sense if we generalized priorities to "startup + shutdown". A bit like interceptors: the priority is more about "nesting" order than about sequential order. So for example a lower priority means startup happens sooner, but shutdown happens later. Or the opposite, whatever makes sense.
2. Make sure priority 0 has special meaning that will stand future evolutions. It could be what gets started up up very first, and shut down very last. It could be application priority.
3. Leave room for Quarkus-defined things that need to start after the app or shut down before it. E.g. Vert.x could start accepting requests after startup observers get called, and stop accepting requests before shutdown observers get called. Theoretically.

With that in mind, something similarly generic to Interceptor.Priority might make sense...

CORE_BEFORE
EXTENSION_BEFORE
APPLICATION
EXTENSION_AFTER
CORE_AFTER

We'd get this startup order:

CORE_BEFORE
EXTENSION_BEFORE
APPLICATION
EXTENSION_AFTER
CORE_AFTER

And this shutdown order:

CORE_AFTER
EXTENSION_AFTER
APPLICATION
EXTENSION_BEFORE
CORE_BEFORE

Obviously calling it "Shutdown priorities" will be a bit weird :]

But... would that fit what you're trying to do with shutdown?

Ah, also... I suspect this is something that needs discussing on the quarkus-dev mailing list. Let's plan that on Zulip.

[mmusgrov]

+1 I think ordering the transaction recovery system early during startup and late during shutdown is needed (for example it should help with #35839 (comment)

[yrodiere]

FYI @mmusgrov Marko changed the approach completely, to only address the specific case of Hibernate.

This follows the discussion on quarkus-dev: https://groups.google.com/g/quarkus-dev/c/CMq7i4EZMXY/m/vy956nJjBwAJ

EDIT: Sorry, wrong link -- I fixed it.

[quarkus-bot]

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit f30b4ed.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit f30b4ed.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.