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

Restart process with name will restart unexpected processes. #558

Closed
sailxjx opened this issue Jul 7, 2014 · 6 comments
Closed

Restart process with name will restart unexpected processes. #558

sailxjx opened this issue Jul 7, 2014 · 6 comments

Comments

@sailxjx
Copy link
Contributor

sailxjx commented Jul 7, 2014

This issue is migrate from #307

pm2 restart path|scriptname command exists for a long time, recently I found some issues with this command, for example:

I have a node service (and others) named 'striker' on my server, the startup script is something like '/usr/local/repos/striker/bin/striker', and I deployed two version of this script, which one is 'release' versions and another is 'general availability' version.

When I start these two service with pm2, it works well.

┌────────────┬────┬─────────┬──────┬────────┬──────┬───────────┬────────┬───────────┬───────────────────────────────────────────────┐
│ App Name   │ id │ mode    │ PID  │ status │ port │ Restarted │ Uptime │    memory │ err logs                                      │
├────────────┼────┼─────────┼──────┼────────┼──────┼───────────┼────────┼───────────┼───────────────────────────────────────────────┤
│ striker-ga │ 0  │ cluster │ 6473 │ online │      │         0 │ 3s     │ 22.684 MB │ /Users/tristan/.pm2/logs/striker-ga-err-0.log │
├────────────┼────┼─────────┼──────┼────────┼──────┼───────────┼────────┼───────────┼───────────────────────────────────────────────┤
│ striker    │ 1  │ cluster │ 6481 │ online │      │         0 │ 0s     │ 12.383 MB │ /Users/tristan/.pm2/logs/striker-err-1.log    │
└────────────┴────┴─────────┴──────┴────────┴──────┴───────────┴────────┴───────────┴───────────────────────────────────────────────┘

When I restart the release version by name, pm2 restart striker, it will restart both two scripts unexpected.

┌────────────┬────┬─────────┬──────┬────────┬──────┬───────────┬────────┬───────────┬───────────────────────────────────────────────┐
│ App Name   │ id │ mode    │ PID  │ status │ port │ Restarted │ Uptime │    memory │ err logs                                      │
├────────────┼────┼─────────┼──────┼────────┼──────┼───────────┼────────┼───────────┼───────────────────────────────────────────────┤
│ striker-ga │ 0  │ cluster │ 6975 │ online │      │         1 │ 0s     │ 20.770 MB │ /Users/tristan/.pm2/logs/striker-ga-err-0.log │
├────────────┼────┼─────────┼──────┼────────┼──────┼───────────┼────────┼───────────┼───────────────────────────────────────────────┤
│ striker    │ 1  │ cluster │ 6976 │ online │      │         1 │ 0s     │ 11.992 MB │ /Users/tristan/.pm2/logs/striker-err-1.log    │
└────────────┴────┴─────────┴──────┴────────┴──────┴───────────┴────────┴───────────┴───────────────────────────────────────────────┘

I found this issue was caused by 'God/Methods.findByName' function. I wonder how many people will use restart command with path or script name. So I removed find by path/script name in findByName function, just keep it simple and clear. What's your opinion?

Cheers!

@soyuka
Copy link
Collaborator

soyuka commented Jul 7, 2014

Quoting for ease:
https://github.com/Unitech/pm2/blob/master/lib/God/Methods.js#L82

I agree that it should only test the "name" not the path basename. I had this issue once and had to use id's instead of name.

 God.findByName = function(name) {
    var db = God.clusters_db;
    var arr = [];

    for (var key in db) {
      if (God.clusters_db[key].pm2_env.name == name) {
        arr.push(db[key]);
      }
    }
    return arr;
  };

@Unitech thoughts?

If we wanted to "restart by path" the check should be done on the whole path like this to be accurate:

God.clusters_db[key].pm2_env.pm_exec_path == name

@soyuka soyuka added bug labels Jul 7, 2014
@Unitech
Copy link
Owner

Unitech commented Jul 8, 2014

It was just a convenient way for beginners to restart their scripts. I didn't got so many complaints about this "feature", so if someone else complain about it, we will patch this

@rlidwka
Copy link
Collaborator

rlidwka commented Jul 10, 2014

I thought about that a bit, and we could change it so pm2 restart something can treat "something" as a filesystem path if it contains slashes and as a name otherwise.

So pm2 restart ./striker and pm2 restart bin/striker would restart by path, and pm2 restart striker would restart by name.

@soyuka
Copy link
Collaborator

soyuka commented Jul 10, 2014

And what if my name contains a "/"? Stupid but might lead to another issue ^^:

pm2 start myapp/1.2b
pm2 start myapp/2.3a

@rlidwka
Copy link
Collaborator

rlidwka commented Jul 10, 2014

pm2 start myapp/1.2b

This would start an application with the name 1.2b. So by default it should be fine.

If someone intentionally names his app myapp/1.2b, pm2 would call him a moron as politely as possible. :)

@sailxjx
Copy link
Contributor Author

sailxjx commented Jul 11, 2014

Just use path.resolve(name) and compare to pm_exec_path will help.
If the user use this command where he start the process, pm2 will restart the process in the correct path.

$ cd your/app/path && pm2 start app.js
$ cd your/app/path && pm2 restart app.js
$ cd your/app && pm2 restart path/app.js

But user may use the restart in anywhere, so this time pm2 just treat the name as process name.

$ cd your/app/path && pm2 start app.js -n app
$ cd ~ && pm2 restart app

The code will be

God.clusters_db[key].pm2_env.pm_exec_path == path.resolve(name) || 
God.clusters_db[key].pm2_env.name == name

How about this solution? :)

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

No branches or pull requests

4 participants