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

Make Server class octoprint_daemon-aware, make sure terminated() gets… #1330

Merged
merged 1 commit into from May 12, 2016

Conversation

Projects
None yet
2 participants
@kevans91
Copy link
Contributor

commented May 9, 2016

What does this PR do and why is it necessary?

This PR is for cleaning up the pidfile of a daemonized instance upon termination. This is my proposed solution to address issue #1324

How was it tested? How can it be tested by the reviewer?

octoprint --daemon start --pid /tmp/octoprint.pid
kill -TERM `cat /tmp/octoprint.pid`
ls -l /tmp/octoprint.pid

What are the relevant tickets if any?

#1324

Screenshots (if appropriate)

Further notes

This seems like the necessary solution for this case. The Server class handles clean-up and termination, so it seems to make the most sense to make it octoprint_daemon-aware and force a clean-up of the Daemon instance. The exit() is no longer needed, as the termination should otherwise be handled as it normally is (now).

This was previously created as #1328, but this iteration addresses the concern by @foosel that we should be cleaning up the pidfile later on in termination. This iteration moves the octoprint daemon cleanup to on_shutdown() instead, just before printing 'Goodbye!', and mentions in the log that it's cleaning up the pidfile. This does mean that we're no longer cleaning up the pidfile strictly on SIGTERM, but that's probably okay because if _octoprint_daemon is not None then we really need to clean up after it if we can.

Make Server class octoprint_daemon-aware, make sure terminated() gets…
… invoked on it upon SIGTERM and update respective test
@kevans91

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2016

I should also note here that this may be better for future cases, too- in the case that there's other options (than SIGTERM) for termination of a daemonized OctoPrint, this cleanup should always happen as long as we're exiting gracefully.

@foosel foosel merged commit 75f8fac into foosel:devel May 12, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@foosel

This comment has been minimized.

Copy link
Owner

commented May 12, 2016

Perfect, thank you :) I'm also going to backport this to the maintenance branch so it will be included (and #1324 fixed) in 1.2.12.

@kevans91

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2016

Oooo, thanks! =)

Looks like it was a pretty straightforward backport -- just needs the two self._logConf and self parameters added to https://github.com/foosel/OctoPrint/blob/maintenance/src/octoprint/__init__.py#L27 =)

Thank you!

@kevans91 kevans91 deleted the kevans91:devel-pid_cleanup2 branch May 15, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.