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

Properly kill dev api-server #11691

Merged
merged 4 commits into from
Oct 14, 2024
Merged

Properly kill dev api-server #11691

merged 4 commits into from
Oct 14, 2024

Conversation

callingmedic911
Copy link
Member

Changes are:

  • await the process where we kill the server process when restarting. So that we don't race the new instance of httpServer with existing ones (while being killed).
  • try to gracefully kill the api server, if it's not killed within 2sec, SIGTERM it.
  • unrelated: moved a comment to the right place. I accidentally moved it wrong place while refactoring in the last PR.

@callingmedic911 callingmedic911 added the release:fix This PR is a fix label Oct 9, 2024
@callingmedic911 callingmedic911 added this to the next-release-patch milestone Oct 9, 2024
callingmedic911
callingmedic911 commented Oct 10, 2024
View reviewed changes
.changesets/11691.md Outdated Show resolved Hide resolved
@callingmedic911 callingmedic911 self-assigned this Oct 10, 2024
peterp
peterp reviewed Oct 10, 2024
View reviewed changes
packages/api-server/src/serverManager.ts Outdated
new Promise<void>((resolve) => {
console.log(
chalk.yellow(
'API server did not exit within 2 seconds, forcefully closing it.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: This message seems inaccurate? This is the first attempt at closing the API server, so maybe we should change it to something like "Shutting down API server."

Copy link
Member Author

@callingmedic911 callingmedic911 Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Whoops, that's a left-out-test-log. Since I could not repro the delayed kill, I wanted to see what "yellow" looked like, so I moved up to the "normal" case.

Thanks for catching it!

@dthyresson
Copy link
Contributor

@peterp are we ok to merge?

@Josh-Walker-GM
Copy link
Collaborator

Don't wait for me. If @peterp is good with this then it's good with me.

@callingmedic911
Copy link
Member Author

Peter already approved it. So, I'll pull the trigger here. :D

@callingmedic911 callingmedic911 merged commit b8cd6aa into main Oct 14, 2024
50 checks passed
@callingmedic911 callingmedic911 deleted the aditya/kill-api-server-properly branch October 14, 2024 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peterp peterp peterp approved these changes

@dthyresson dthyresson Awaiting requested review from dthyresson

@Josh-Walker-GM Josh-Walker-GM Awaiting requested review from Josh-Walker-GM

Assignees

@callingmedic911 callingmedic911

Labels
release:fix This PR is a fix
Projects
Bighorn Development
Status: Done
Milestone
next-release-patch
Development

Successfully merging this pull request may close these issues.
4 participants
@callingmedic911 @dthyresson @Josh-Walker-GM @peterp