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

Kill existing debug processes before starting the debug server. #3484

Merged
merged 3 commits into from Feb 7, 2020

Conversation

@jim
Copy link
Contributor

jim commented Feb 6, 2020

Description

For whatever reason, the server processes don't exit cleanly when run through delve. This PR kills any running processes on the server ports so that the debug server can start.

Reviewer Notes

I also copied the target dependencies from the normal server_run as I hit a situation the debug server was failing because one of those dependencies was out of date.

Setup

Add any steps or code to run in this section to help others prepare to run your code:

$ make server_run_debug

# then exit out of the debugger

$ make server_run_debug

The second invocation should work without an error about a port already bring in use.

@jim jim requested review from chrisgilmerproj, tinyels, lynzt and gidjin Feb 6, 2020
Copy link
Contributor

chrisgilmerproj left a comment

🚀

Makefile Outdated
server_run_debug: check_log_dir ## Debug the server
server_run_debug: .check_hosts.stamp .check_go_version.stamp .check_gopath.stamp .check_node_version.stamp check_log_dir build/index.html server_generate db_dev_run ## Debug the server
lsof -t -i :8080 | xargs kill -9
lsof -t -i :9443 | xargs kill -9

This comment has been minimized.

Copy link
@tinyels

tinyels Feb 6, 2020

Contributor

This is ok if we believe that no one ever works on anything else that might use those ports.
It would be annoying if it killed something I didn't want to kill.

This comment has been minimized.

Copy link
@gidjin

gidjin Feb 6, 2020

Contributor

I agree with Erin, it could be annoying to kill a process without knowing it. Maybe it's worth just printing out the results of the lsof and a note saying can't start because the ports are taken. Or asking for confirmation before the kill command.

@jim jim requested a review from tinyels Feb 7, 2020
@jim

This comment has been minimized.

Copy link
Contributor Author

jim commented Feb 7, 2020

@tinyels @gidjin I agree with you, and added a script to ask before killing processes.

@tinyels
tinyels approved these changes Feb 7, 2020
Copy link
Contributor

tinyels left a comment

This is great!

@jim jim merged commit 13b098a into master Feb 7, 2020
15 checks passed
15 checks passed
ci/circleci: acceptance_tests_experimental Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_local Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_staging Your tests passed on CircleCI!
Details
ci/circleci: build_app Your tests passed on CircleCI!
Details
ci/circleci: build_migrations Your tests passed on CircleCI!
Details
ci/circleci: build_storybook_app Your tests passed on CircleCI!
Details
ci/circleci: build_tasks Your tests passed on CircleCI!
Details
ci/circleci: build_tools Your tests passed on CircleCI!
Details
ci/circleci: check_generated_code Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: integration_tests Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_golang Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_yarn Your tests passed on CircleCI!
Details
ci/circleci: pre_test Your tests passed on CircleCI!
Details
ci/circleci: server_test Your tests passed on CircleCI!
Details
@jim jim deleted the jb-kill-processes-debug branch Feb 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.