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

[fix] cron should gracefully reload process #5636

Open
titanism opened this issue Jul 10, 2023 · 3 comments
Open

[fix] cron should gracefully reload process #5636

titanism opened this issue Jul 10, 2023 · 3 comments

Comments

@titanism
Copy link

What's going wrong?

Disclosure: We haven't seen this issue on GitHub Issues nor Pull Requests tabs here on GitHub at https://github.com/Unitech/pm2/issues/new, therefore we are filing this is as a new issue. There are related discussion threads on StackOverflow however, see the references below.

Issue: The issue is that the cron-restart option does not call reload, and therefore it does a hard-restart on an existing running application (as opposed to a graceful reload via pm2 reload). The underlying reason for this is the code below which calls God.restartProcessId as opposed to God.reloadProcessId (restart vs reload).

God.restartProcessId({id: pm_id}, function(err, data) {

God.restartProcessId = function(opts, cb) {

God.reloadProcessId = function(opts, cb) {

References:

How could we fix this issue?

Current Workaround: The current workaround is to add the following line to crontab on your server and then modify the paths appropriately. Here are the three steps required to achieve this:

  1. ssh your-server
  2. Run which node and which pm2 to get the full paths to the executables for node and pm2. In the example below we are using n, but your paths may differ slightly, e.g. it may be /usr/local/bin/node and /usr/local/bin/pm2.
  3. Edit crontab using crontab -e and in the editor (e.g. vim) add the following line to the bottom, then save via :wq:
    0 */4 * * * /home/deploy/n/bin/node /home/deploy/n/bin/pm2 reload all > /dev/null 2>&1

How to Fix the Issue: To fix this issue, we'd have to open a pull request that modifies the logic as such:

  God.registerCron = function(pm2_env) {
    if (!pm2_env ||
        pm2_env.pm_id === undefined ||
        !pm2_env.cron_restart ||
        pm2_env.cron_restart == '0' ||
        God.CronJobs.has(God.getCronID(pm2_env.pm_id)))
      return;

    var pm_id = pm2_env.pm_id
    console.log('[PM2][WORKER] Registering a cron job on:', pm_id);

    var job = Cron(pm2_env.cron_restart, function() {
+      God.softReloadProcessId({id: pm_id}, function(err, data) {
-      God.restartProcessId({id: pm_id}, function(err, data) {
        if (err)
          console.error(err.stack || err);
        return;
      });
    });

    God.CronJobs.set(God.getCronID(pm_id), job);
  }
@titanism
Copy link
Author

We are submitting a pull request now to resolve this issue. The intended behavior by default should perform a graceful reload, as the out of the box behavior should be as smooth as possible for end-users. This change should require a major semver version bump, but a minor at the least.

@titanism titanism changed the title [FIX] Cron should gracefully reload process if already running (and restart/start if not already running) [fix] cron should gracefully reload process if already running (and restart/start if not already running) Jul 10, 2023
@titanism titanism changed the title [fix] cron should gracefully reload process if already running (and restart/start if not already running) [fix] cron should gracefully reload process Jul 10, 2023
titanism added a commit to titanism/pm2 that referenced this issue Jul 10, 2023
Per <Unitech#5636>.  This should be at minimum a minor semver bump, but recommended major semver bump.
@titanism
Copy link
Author

See #5637.

@titanism
Copy link
Author

titanism commented Dec 7, 2023

ping @Unitech

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

No branches or pull requests

1 participant