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

feat: world cardinal dev graceful shutdown #12

Conversation

pyrofolium
Copy link
Contributor

Closes: #496

What is the purpose of the change

When you run world-cli cardinal dev then ctrl+C to quit you'll see a wierd error message about how the process already quit then the cli will display the help message.

this occurs because when scott was messing with the cli he used a version of cardinal that hung during shutdown. So he used a force quit to kill it.

Now that cardinal handles shutdown properly and doesn't hang forever during start game the mechanism is now changed to wait 10 seconds and see if the cardinal shutsdown by itself. If it doesn't then force quit.

wait up to 10 seconds for it to quit before force quitting.
@pyrofolium pyrofolium requested review from smsunarto and a team November 3, 2023 23:57
Copy link

codecov bot commented Nov 3, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3e4e3dc) 0.00% compared to head (027329f) 0.00%.

Additional details and impacted files
@@          Coverage Diff          @@
##            main     #12   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files          5       5           
  Lines        284     284           
=====================================
  Misses       284     284           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@smsunarto smsunarto changed the title Brian/world 496 fix cli error message issue when quitting cardinal feat: world cardinal dev graceful shutdown Nov 4, 2023
Copy link
Member

@smsunarto smsunarto left a comment

Choose a reason for hiding this comment

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

lgtm

@smsunarto smsunarto merged commit 537014c into main Nov 4, 2023
7 checks passed
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.

2 participants