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

use npid for pid file support #555

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

Conversation

apmorton
Copy link

as per: #379 (comment)

--silent is useful in a cron job script, like this one I use, in order to prevent repeated log entries about failing to create the pid file

#!/bin/bash

if [ -z $(ps -p $(cat ~/.shout/shout.pid) -o pid=) ]; then
    rm -f ~/.shout/shout.pid
fi

node /home/shout/shout/index.js --silent >> /home/shout/shout.log 2>&1 &

I run the above script with cron every minute in order to check for stale pid files, and then try and start shout.

The stale file check is important, because although the file should be removed on a crash, if the process is killed (SIGKILL), or the server itself crashes, you can be left with a stale pid file which would prevent shout from starting.

// boolean values result in the default file path being used
//
// paths are relative to the 'HOME' folder
//
Copy link
Collaborator

Choose a reason for hiding this comment

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

For consistency with other paths in config (ex: ssl key), this should not be relative to HOME folder (+ it's a limitation).

@JocelynDelalande
Copy link
Collaborator

--silent is useful in a cron job script, like this one I use, in order to prevent repeated log entries about failing to create the pid file

In which cases would that occur ?

@JocelynDelalande
Copy link
Collaborator

@apmorton thanks a lot for your contrib and sorry for my ashamingly-high delay on the review, I will try to be more reactive on the discussion :-)

}

// get the absolute path of the pid file to use
var pidFilePath = path.resolve(Helper.HOME, config.pidFile);
Copy link
Author

Choose a reason for hiding this comment

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

as you can see here @JocelynDelalande, although paths are relative to HOME, I use the resolve function, which respects absolute paths when given.

IE, if you specify pidFile: 'shout.pid' vs pidFile: '/shout.pid' the first will be resolved to the home path, and the second will be used verbatim

@apmorton
Copy link
Author

apmorton commented Jan 8, 2016

If you notice in my example cron script, I always attempt to start shout - I do not check for the existence of the pid file to control starting shout. Perhaps --silent is not required, because you could implement additional checks in the cron job itself before attempting to launch shout.

if shout is already running with the same pid file, and you try to launch it again, you get a pid file creation error message. --silent suppresses this

@Xe
Copy link
Contributor

Xe commented Jan 8, 2016

Have you instead considered systemd instead of relying on a pidfile?

@apmorton
Copy link
Author

apmorton commented Jan 8, 2016

correct me if I am wrong, but systemd daemons can only be configured system-wide, by a root user. I am wrong

With a generic pidfile approach, any user can configure shout without needing to be root.

@Xe
Copy link
Contributor

Xe commented Jan 8, 2016

systemd-user is a thing.

@apmorton
Copy link
Author

apmorton commented Jan 8, 2016

that being said, I don't know much about systemd, which is why it was not my preferred solution.

TL;DR; pidfiles work - so does systemd, something about skinning a cat

@astorije
Copy link
Collaborator

Do we want to have to maintain everything about OS integration?! I mean, we have a Dockerfile, this would add npid information to the config file. I also noticed someone PRing for forever, there were mentions of systemd, ...
I personally use supervisor but I don't want to add supervisor configuration in shout's git repo (since it's personal, I maintain it personally here: https://github.com/astorije/ansible-role-shout).
What about Homebrew then? Or Windows installers?

I'd rather see this repo and project about and shout itself and its features, and let the community come up with integration projects. When some of these reach a nice stable point, we can deem them official if needed.

@apmorton, would it be possible to have this in a repo you control and maintain? Then we can list interesting integration projects in a wiki page, the documentation repo and/or website, ...

What do you guys think?

@apmorton
Copy link
Author

I think having pidfile support is pretty OS agnostic...

In any case, I sent this PR at the request of @JocelynDelalande #379 (comment)

@JocelynDelalande
Copy link
Collaborator

Do we want to have to maintain everything about OS integration?! I mean, we have a Dockerfile, this would add npid information to the config file. I also noticed someone PRing for forever, there were mentions of systemd, ...
I personally use supervisor but I don't want to add supervisor configuration in shout's git repo (since it's personal, I maintain it personally here: https://github.com/astorije/ansible-role-shout).
What about Homebrew then? Or Windows installers?

Offering some decent, light and cross-system PID writing mechanism does not hurt, IMHO, and some init system can integrate of a program that writes its pid to a file itself.

About installers, that's a different question, as we already offer a preferred and cross-OS way : npm installation.

About supporting various init systems (although I think that's not what we should be discussing here), I'm not against a contrib dir including systemd/runnit/OSXinit/whatever init scripts. I do agree that it's something more to maintain, but I'm also sure that it's an important thing for a server software, and we better keep an eye on it (+ it's realy light : one text file for each init system).

What do you guys think?

To sum it up, the pid file creation (this PR) does not goes as far as an init system but allows easier management/scripting around it.

@apmorton I pause technical discussion till we agree on whether or not the feature itself has an interest.

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

Successfully merging this pull request may close these issues.

None yet

4 participants