soyuka / pidusage

Cross-platform process cpu % and memory usage of a PID
MIT License
512 stars 55 forks source link

Can't shutdown server with pidusage interval running #77

Closed gboysko closed 6 years ago

gboysko commented 6 years ago

If you use pidusage in a server application, it will not allow a graceful shutdown as the runInvalidator interval remains running.

How can we tell pidusage to shutdown and close up its interval?

soyuka commented 6 years ago

I'm really not sure I follow up. You can clear your interval with clearInterval. If you're talking about an external application please add more data.

gboysko commented 6 years ago

How would an external application using pidusage clear the interval inside your module? If you have a Server application, it won't shutdown until all of the intervals, timers and socket connections are closed.

soyuka commented 6 years ago

Ohh I misunderstood you're talking about https://github.com/soyuka/pidusage/blob/da1c5fb2480bdf8f871476d79161dac7733b89f3/lib/history.js#L41 !

@simonepri any ideas regarding this?

soyuka commented 6 years ago

@gboysko would my patch fix the problem?

simonepri commented 6 years ago

@soyuka I think the fix is correct but I'm not 100% sure. @gboysko can you give us a simple repro code?

gboysko commented 6 years ago

I'm not sure if beforeExit will be called. The simplest way to reproduce this is a follows:

  1. Create an HTTP Server that accepts incoming requests on a default port (say 3000).
  2. With each incoming call, return the results of calling pidusage for some particular metric.
  3. Handle the SIGINT signal by stopping the HTTP Server.

Does the process exit on its own? Or does the interval that you create in pidusage cause the process to stay running?

gboysko commented 6 years ago

A better option may be to expose a shutdown method on the pidusage object that can be used to close the interval and anything else that would cause a shutdown to hang.

soyuka commented 6 years ago

Mhh I've investigated a bit, the http server isn't really useful to try this out but having a simple code like this will do:

var pidusage = require('../')

pidusage(process.pid, function (err, stat) {
  if (err) {
    throw err
  }
})

Then, wait until the process exits by itself.

@gboysko I'm really not sure that I want to consider this as a bug because in fact the event loop will get free'd eventually. To reduce this time you could specify the option maxage, for example:

pidusage(1234, {maxage: 2000}, cb)

I've also added a clean method (see #78) to the API in case you still want to free the event loop.

gboysko commented 6 years ago

Thanks, @soyuka. I'm not sure what maxage is controlling. Is it how often the interval runs? And in your code, are you periodically clearing the interval and restarting it? If so, why not simply use a timeout? I have a server application that simply shares statistics about its memory and CPU usage. How would changing the maxage parameter affect the statistics that I'm publishing about my server process?

I'm wondering whether it is worth using pidusage as I've gotten burned in the past with this module and instabilities. Maybe it would easiest to simply omit cpu usage and simply report using Node.js's own process.cpuUsage(). I think that using the clean method is probably the safest thing for me. I'll try that next...

soyuka commented 6 years ago

I'm not sure what maxage is controlling. Is it how often the interval runs?

It controls how long the data stays in memory. Indeed, while computing cpu usage for example, we need to keep things in memory.

And in your code, are you periodically clearing the interval and restarting it? If so, why not simply use a timeout?

Yes, a timeout won't be resolving the issue and it's also why your process will eventually end up by itself (maxage is set to 60s by default so it can take a while).

How would changing the maxage parameter affect the statistics that I'm publishing about my server process?

Values might be less accurate, especially if you call pidusage less then it clears its history.

I'm wondering whether it is worth using pidusage as I've gotten burned in the past with this module and instabilities. Maybe it would easiest to simply omit cpu usage and simply report using Node.js's own process.cpuUsage(). I think that using the clean method is probably the safest thing for me. I'll try that next...

It should be mentioned in the readme but if you need cpu + memory and that your service is in nodejs you should definitely use process.cpuUsage() and process.memoryUsage(). This module is only useful when you only have a pid and need these data. If your processes are nodejs child processes you can even go through IPC to get back your children's cpuUsage and memoryUsage data, which I definitely recommend!

tldr: if you're only using nodejs, don't use this module and prefer process.cpuUsage and process.memoryUsage

gboysko commented 6 years ago

Thanks @soyuka. My processes (both the master and child processes) are in Node.js and I'll probably go towards using process.cpuUsage() directly. Just don't have an easy way to get that into a CPU %, but perhaps it is not worth it.

soyuka commented 6 years ago

CPU % is really an abstract thing, basically it represents ticks / time. This quick one-liner might help. By the way this is using IPC to get the cpu/memory of a child process :).

gboysko commented 6 years ago

Thanks, @soyuka. I found a similar approach for computing the CPU percentage that seems to yield good results when compared with the OS monitor. And yes, I'm using IPC to capture usage in each worker process and aggregate them.

We can close this issue, if you like. I'm not going to use pidusage as all of my processes are Node.js-based.