fix: should stop app when baseDir is symlink #12
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
atian25
reviewed
Nov 14, 2017
| @@ -37,7 +37,7 @@ class StopCommand extends Command { | |||
| // node /Users/tz/Workspaces/eggjs/egg-scripts/lib/start-cluster {"title":"egg-server","workers":4,"port":7001,"baseDir":"/Users/tz/Workspaces/eggjs/test/showcase","framework":"/Users/tz/Workspaces/eggjs/test/showcase/node_modules/egg"} | |||
| let processList = yield this.helper.findNodeProcess(item => { | |||
| const cmd = item.cmd; | |||
| return cmd.includes(this.serverBin) && cmd.includes(`"baseDir":"${baseDir}"`); | |||
| return cmd.includes('start-cluster'); | |||
There was a problem hiding this comment.
这里之前也是纠结要不要严谨一点，否则 window 的用 find-process 就很好搞了。
这里不判断目录的话，会 kill 掉其他 egg 应用吧，有些场景是一个机器多个 egg，没 docker 隔离的。
There was a problem hiding this comment.
隔离的到时可以加个唯一 id 做标识，我们应该不推崇这种方式，支持部署脚本支持一下。
There was a problem hiding this comment.
如果真是软连接，basename 也不一定一样
There was a problem hiding this comment.
你指的是worker的监听端口会随着重启而不断变化么？ 不过master的端口是固定的吧
There was a problem hiding this comment.
那监听0.0.0.0：7001端口的node进程（并且包含多个子进程）不是master进程？
There was a problem hiding this comment.
node 9.2 支持 process.ppid
nodejs/node#16839
There was a problem hiding this comment.
master - agent - workerS(7001)
|
后面可以用 title 做标识，如果开发者指定了才设置，stop 的时候如果指定了则根据这个过滤 |
|
|
也就是：
|
popomore
added a commit
that referenced
this pull request
Nov 14, 2017
popomore
added a commit
that referenced
this pull request
Nov 14, 2017
|
嗯，主要是要确定下，我们的寻找 pid 逻辑是怎么样才合理，我之前的做法是太严格了导致 win 那边反斜杠很难处理。 |
|
看下我给的那个文档，或者看 egg-cluster 源码就知道了。
agent 和 worker 都是 master 的子进程，自然是 3 个
发自我的 iPhone
… 在 2017年11月15日，18:09，waiting ***@***.***> 写道：
@waitingsong commented on this pull request.
In lib/cmd/stop.js:
> @@ -37,7 +37,7 @@ class StopCommand extends Command {
// node /Users/tz/Workspaces/eggjs/egg-scripts/lib/start-cluster {"title":"egg-server","workers":4,"port":7001,"baseDir":"/Users/tz/Workspaces/eggjs/test/showcase","framework":"/Users/tz/Workspaces/eggjs/test/showcase/node_modules/egg"}
let processList = yield this.helper.findNodeProcess(item => {
const cmd = item.cmd;
- return cmd.includes(this.serverBin) && cmd.includes(`"baseDir":"${baseDir}"`);
+ return cmd.includes('start-cluster');
我用 --worker=2 启动，一共4个进程。 pid：6872（监听7002端口）是agent进程，那么怎么会有3个子进程呢
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
你上面说“master 不提供 http 服务，无端口。” |
|
提供 的 http 服务的当然是 worker 啦。先看下文档吧
发自我的 iPhone
… 在 2017年11月15日，18:16，waiting ***@***.***> 写道：
你上面说“master 不提供 http 服务，无端口。”
那么请问监听7002主端口的6872进程是master还是agent进程？
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
发布系统的应用代码会放在 一个发布 id 目录下，然后链接到固定目录，发布会切换软链接 |
This was referenced Nov 24, 2017
popomore
added a commit
that referenced
this pull request
Nov 30, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checklist
npm testpassesAffected core subsystem(s)
Description of change