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

Added feature to reopen log files #16

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

sebastianhoitz
Copy link

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
Copy link
Member

@sebastianhoitz Can you add tests for this please?

@colinmollenhour
Copy link

+1

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

@indexzero
Copy link
Member

@sebastianhoitz Ping. Any progress on those tests?

@sebastianhoitz
Copy link
Author

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
Copy link
Author

@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?

…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 [email protected]
  [dist fix] Support [email protected]
  [dist] Version bump. 1.2.0
  [fix test] Allow for commands that have `node` in their name. Fixes foreversd#7
  [fix] Set `Monitor.prototype.inspect` to null to `util.inspect` doesnt return "undefined". Fixes foreversd#2.
  [fix] Dont spawn `kill` for Windows compatibility. Fixes foreversd#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 foreversd#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
  ...
@colinmollenhour
Copy link

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
Copy link
Member

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

@colinmollenhour
Copy link

@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

@adrian-ludwig
Copy link

+1

@leonerd
Copy link

leonerd commented Apr 26, 2017

Is there any hint of progress on this? We'd quite like this feature.

Also, why was SIGUSR2 picked? The SIGHUP signal is far more usual for this sort of activity; programs like logrotate will send that by default.

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

Successfully merging this pull request may close these issues.

5 participants