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.

Sign up for GitHub

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

Jump to bottom

Issue #8088 Add STOP.EXIT System property to configure ShutdownMonitor.exitVm #8089

Merged
merged 3 commits into from Jun 7, 2022
Merged

Issue #8088 Add STOP.EXIT System property to configure ShutdownMonitor.exitVm #8089

merged 3 commits into from Jun 7, 2022

Conversation

janbartel
Copy link
Contributor

Closes #8088

@janbartel janbartel added this to In progress in Jetty 9.4.47 - 🧊 FROZEN 🥶 via automation Jun 1, 2022
@janbartel janbartel self-assigned this Jun 1, 2022
@janbartel janbartel requested a review from gregw June 1, 2022 04:44
@janbartel
Copy link
Contributor Author

@gregw note that even with the new STOP.EXIT=false System property - which prevents calling System.exit() - the vm will still exit after the server is stopped. Probably this is because there are only daemon threads left. Pretty sure we used to call server.join() somewhere to prevent that happening, but it doesn't seem to be the case any more.

@joakime joakime moved this from In progress to Review in progress in Jetty 9.4.47 - 🧊 FROZEN 🥶 Jun 1, 2022
joakime
joakime previously approved these changes Jun 1, 2022
View reviewed changes
Jetty 9.4.47 - 🧊 FROZEN 🥶 automation moved this from Review in progress to Reviewer approved Jun 1, 2022
@gregw gregw added the Sponsored This issue affects a user with a commercial support agreement label Jun 1, 2022
olamy
olamy previously approved these changes Jun 1, 2022
View reviewed changes
@janbartel janbartel dismissed stale reviews from olamy and joakime via 7339978 June 2, 2022 04:14
Jetty 9.4.47 - 🧊 FROZEN 🥶 automation moved this from Reviewer approved to Review in progress Jun 2, 2022
@janbartel janbartel requested review from olamy and joakime June 2, 2022 04:14
olamy
olamy previously approved these changes Jun 2, 2022
View reviewed changes
Jetty 9.4.47 - 🧊 FROZEN 🥶 automation moved this from Review in progress to Reviewer approved Jun 2, 2022
Jetty 9.4.47 - 🧊 FROZEN 🥶 automation moved this from Reviewer approved to Review in progress Jun 2, 2022
joakime
joakime reviewed Jun 2, 2022
View reviewed changes
jetty-server/src/test/java/org/eclipse/jetty/server/ShutdownMonitorTest.java
*
* Undisable to test individually as needed.
*/
@Disabled
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Perhaps these exit VM tests should be in test-distribution ?

Jetty 9.4.47 - 🧊 FROZEN 🥶 automation moved this from Review in progress to Reviewer approved Jun 5, 2022
@janbartel janbartel merged commit 208b150 into jetty-9.4.x Jun 7, 2022
Jetty 9.4.47 - 🧊 FROZEN 🥶 automation moved this from Reviewer approved to Done Jun 7, 2022
@janbartel janbartel deleted the jetty-9.4.x-8088-shutdownmonitor-exitVm branch June 7, 2022 03:09
janbartel added a commit that referenced this pull request Jun 7, 2022
@janbartel
Issue #8088 Add STOP.EXIT System property to configure ShutdownMonito…
183b3c0 
…r.exitVm (#8089)

* Issue #8088 Add STOP.EXIT System property to configure ShutdownMonitor.exitVM

* Ensure missing STOP.EXIT doesn't override default exitVm=true

* Disable another test
@joakime joakime linked an issue Jun 7, 2022 that may be closed by this pull request
Add option to configure exitVm on ShutdownMonitor from System properties #8088
Closed
janbartel added a commit that referenced this pull request Jun 8, 2022
@janbartel
Issue #8088 Add STOP.EXIT System property to configure ShutdownMonito…
d725855 
…r.exitVm (#8133)

* Issue #8088 Add STOP.EXIT System property to configure ShutdownMonitor.exitVm (#8089)

* Issue #8088 Add STOP.EXIT System property to configure ShutdownMonitor.exitVM

* Ensure missing STOP.EXIT doesn't override default exitVm=true

* Disable another test

* Disable test that might not work, depending on test execution order.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joakime joakime joakime left review comments

@gregw gregw gregw approved these changes

@olamy olamy olamy left review comments

Assignees

@janbartel janbartel

Labels
Sponsored This issue affects a user with a commercial support agreement
Projects
No open projects
Jetty 9.4.47 - 🧊 FROZEN 🥶
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add option to configure exitVm on ShutdownMonitor from System properties
4 participants
@janbartel @olamy @gregw @joakime