Skip to content
This repository has been archived by the owner on Nov 8, 2022. It is now read-only.

Add PID file support #1733

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add PID file support #1733

wants to merge 1 commit into from

Conversation

cce
Copy link
Contributor

@cce cce commented Sep 14, 2017

Fixes #878 by allowing for systemd, init scripts, etc, to be able to better manage snapteld and send it kill or restart signals.

Summary of changes:

  • Added --pid-file flag and pid_file config file option to snapteld
  • Added PIDFile to snapteld Config struct.

Testing done:

  • Added PIDFile to command-line flags testing in snapteld_test.go.

@intelsdi-x/snap-maintainers

fmt.Fprintf(f, "%d\n", os.Getpid())
f.Close()
defer func() {
err := os.Remove(cfg.PIDFile)
Copy link

Choose a reason for hiding this comment

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

Sorry, I have a stupid question to ask.
Why delete the PIDFile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most servers are expected to delete their PID file when the server shuts down — for example, Apache removes its PID file when shutting down.

However, since snapteld calls os.Exit() when it shuts down, I recently realized that this deferred function will not be executed, since os.Exit shuts down immediately without calling deferred functions.

Copy link

Choose a reason for hiding this comment

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

@cce
Thanks for your explanation!
Yes, I think the the actual behavior is not your meaning.
Could you please update this PR according your design when you are free?

@WangJialei-A
Copy link
Contributor

@katarzyna-z I think it is better to handle the PID file in the service script, but not in snapteld.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants