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

Add shutdown listener, which shuts down the pool on JVM exit #88

Merged
merged 1 commit into from
Mar 1, 2024

Conversation

rPraml
Copy link
Contributor

@rPraml rPraml commented Jan 3, 2024

Refering to the problem here ebean-orm/ebean-migration#153 I've tried to implement a shutdown listener.

The problem is, that H2 needs ~4 seconds to shut down open connections when the JVM exits

@rPraml
Copy link
Contributor Author

rPraml commented Feb 7, 2024

@rbygrave I plan to contiune the work on the serviceloader in about 2-3 weeks

can you take a look at this and maybe at ebean-orm/ebean-migration#153 in the meantime?

@rob-bygrave
Copy link
Collaborator

I'm not sure if this is a good idea at the moment.

@rob-bygrave
Copy link
Collaborator

Hi @rPraml when do you think it is a good idea to use this shutdown hook?

In general we do not want this when using Ebean Database because shutdown ordering is important and io.ebean.Database controls the order of shutdown and currently does the correct thing in terms of shutdown order.

The only time I'd expect people to use this is when they are using ebean-datasource without using io.ebean.Database. Is that your expectation? If so, why is this defaulting to be on?

The counter argument is that as long as new requests for connections are not being made then shutdown() can be requested and it will wait for busy connections to complete. I presume this is the thinking behind making this true by default?

rbygrave added a commit that referenced this pull request Mar 1, 2024
- Default this to off for now
- Adjust to support DataSourceBuilder (preferred going forward)
- Change to log at INFO for when the hook is run
- Adjust the test to include an already shutdown pool
@rbygrave rbygrave merged commit 3c584d6 into ebean-orm:master Mar 1, 2024
1 check passed
@rbygrave rbygrave added this to the 8.12 milestone Mar 1, 2024
@rbygrave
Copy link
Member

rbygrave commented Mar 1, 2024

Going with leaving this off by default. I believe we could make this on by default ... and then have ebean turn it off for it's use case (where ebean wants to control the shutdown order and specifically shutdown the pool after any background tasks have completed).

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

Successfully merging this pull request may close these issues.

3 participants