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

修复『全局命令cnpmjs.org stop不稳定』问题 #1220

Merged
merged 1 commit into from Sep 5, 2017

Conversation

Projects
None yet
4 participants
@stoneChen
Contributor

stoneChen commented Sep 1, 2017

首先,cnpmjs.org很棒,感谢cnpm团队!
我是通过全局安装npmjs.org模块方式部署的,所以就需要通过cnpmjs.org start/stop启动/停止服务,经排查,原因是:
bin/cli.js的stop方法中的treekill为异步方法,而在start方法中是同步调用stop的,导致传入的回调(删除pid文件)几乎必然(递归杀进程)比start方法中的『写入pid操作』要晚,最终的结果是,服务起来了,但是pid文件丢失了。

而通过源码部署,npm start调用的是shell脚本,而且是通过ps ax查询进程号,没有异步的过程,所以没有问题。

现已将stop方法改为返回Promise,解决此问题。
请考虑合并~

chenshidong
@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Sep 1, 2017

@stoneChen, thanks for your PR! By analyzing the history of the files in this pull request, we identified @fengmk2 and @gniavaj to be potential reviewers.

mention-bot commented Sep 1, 2017

@stoneChen, thanks for your PR! By analyzing the history of the files in this pull request, we identified @fengmk2 and @gniavaj to be potential reviewers.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Sep 1, 2017

Codecov Report

Merging #1220 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1220   +/-   ##
======================================
  Coverage    87.7%   87.7%           
======================================
  Files          88      88           
  Lines        3668    3668           
  Branches      699     699           
======================================
  Hits         3217    3217           
  Misses        451     451

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c019de...17df5f0. Read the comment docs.

codecov-io commented Sep 1, 2017

Codecov Report

Merging #1220 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1220   +/-   ##
======================================
  Coverage    87.7%   87.7%           
======================================
  Files          88      88           
  Lines        3668    3668           
  Branches      699     699           
======================================
  Hits         3217    3217           
  Misses        451     451

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c019de...17df5f0. Read the comment docs.

@fengmk2

This comment has been minimized.

Show comment
Hide comment
@fengmk2

fengmk2 Sep 5, 2017

Member

Thanks!

Member

fengmk2 commented Sep 5, 2017

Thanks!

@fengmk2 fengmk2 merged commit 43ffa99 into cnpm:master Sep 5, 2017

4 of 5 checks passed

Node Security 1 vulnerability found
Details
codecov/patch Coverage not affected when comparing 6c019de...17df5f0
Details
codecov/project 87.7% remains the same compared to 6c019de
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk No new issues
Details

fengmk2 added a commit that referenced this pull request Sep 5, 2017

@fengmk2

This comment has been minimized.

Show comment
Hide comment
@fengmk2

fengmk2 Sep 5, 2017

Member

@stoneChen 我将这个 bugfix 也修复到 2.x 版本。

Member

fengmk2 commented Sep 5, 2017

@stoneChen 我将这个 bugfix 也修复到 2.x 版本。

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