Added feature to reopen log files #16

Open
wants to merge 5 commits into from

4 participants

@sebastianhoitz

This functionality is especially useful when you want to implement a logrotate functionality with native tools like logrotate.

Example implementation

After rotating the logs, you could send your process a custom SIGUSR2 signal:

$ kill -SIGUSR2 **PID**

Your node.js app could handle this request like this:

process.on("SIGUSR2", function() {
  console.log("Received SIGUSR2 signal. Stopping application");
  process.exit(13); // Notice this exit code. This tells forever that the script wants to reopen its log files.
});

Forever now checks the exit code of the child process that died. When it is 13, it creates a new filedescriptor for the process.stdout stream.

Improvement

While I feel that the event-based notification within forever is pretty good, I don't like the communication between the child-process and forever yet.

However I could not get the child to notify forever-monitor in a better way.

I was thinking about letting the monitor process know via sending a SIGUSR2 command to the process from my app like this:

process.on("SIGUSR2", function() {
  console.log("Received SIGUSR2 signal. Stopping application");
  process.kill(PARENT_MONITOR_PID, "SIGUSR2"); // Notice this exit code. This tells forever that the script wants to reopen its log files.
});

however then I have to somehow pass that Parent monitor PID to my application, which seems kinda bad, too. And because the child processes aren't forked, but rather spawned in a whole new process, they have no way to communicate with the parent process via process.send. Would it be possible to change the way forever spawns new childs so that they can send data to the parent monitor process?


So yes, there is room for improvement. But because of lack of knowledge with forever I was not able to do this any better. Please let me know what you think!

@indexzero
foreverjs member

@sebastianhoitz Can you add tests for this please?

@colinmollenhour

+1

But, does the old fd not need to be closed?

@indexzero
foreverjs member

@sebastianhoitz Ping. Any progress on those tests?

@sebastianhoitz

Hi!

Sorry, I just noticed your comment about the tests last week. I will look into writing tests for this this week so that we can get the pull request merged in! :)

@sebastianhoitz

@indexzero Ok, I now added a small unit test that checks whether the "reopenLogs" event is emitted. However, I would like to add more in-depth checks (see the commented code if the files exist). What would be a good way to do this in your opinion?

@colinmollenhour I now close the old file descriptors. Is this ok now?

@sebastianhoitz sebastianhoitz Merge branch 'master' of https://github.com/nodejitsu/forever-monitor
…into feature/reopenlogs

* 'master' of https://github.com/nodejitsu/forever-monitor: (23 commits)
  [dist] Version bump. 1.2.1
  [dist test] Update travis to test node@0.10.x
  [dist fix] Support node@0.10.x
  [dist] Version bump. 1.2.0
  [fix test] Allow for commands that have `node` in their name. Fixes #7
  [fix] Set `Monitor.prototype.inspect` to null to `util.inspect` doesnt return "undefined". Fixes #2.
  [fix] Dont spawn `kill` for Windows compatibility. Fixes #27.
  [minor] Style compliance.
  added conditional running check to prevent multiple re-starts
  Tweaking the childData so tests pass
  reset max counter, return useful data from crashed processes, misc. supporting logic
  [fix] Correctly set `watchIgnoreDotFiles`. Fixes #25.
  [minor test fix] JSHint compliance. Refeactor `test/plugins/watch-test.js` to avoid the possibility of a race.
  remove console.log stmt
  Test added for sending a child process a message
  Add send message support to a forked child process
  fix handling dir wildcards in .foreverignore
  [fix] Fix tests when all env vars are not displayed in a single stdout message.
  Fixed setting of signal for 'kill' command
  Added custom child stop signal support
  ...
6728411
@colinmollenhour

I'm not 100% sure they even need to be closed, but it seems like they would need to be. Should be possible to test with lsof to see if there are extra file handles after a rotate.

@indexzero
foreverjs member

@sebastianhoitz what's the significance of exit code 13?

@colinmollenhour

@indexzero It is the signal code for "Broken Pipe" which would occur if the log file was moved or deleted.

http://people.cs.pitt.edu/~alanjawi/cs449/code/shell/UnixSignals.htm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment