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: agent should exit on disconnect event whatever master kill with SIGKILL #27

Merged
merged 4 commits into from May 28, 2017

Conversation

Projects
None yet
5 participants
@fengmk2
Copy link
Member

fengmk2 commented May 25, 2017

closes eggjs/egg#960

Checklist
  • npm test passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)
Description of change

@fengmk2 fengmk2 added the bug label May 25, 2017

@fengmk2 fengmk2 requested a review from popomore May 25, 2017

@mention-bot

This comment has been minimized.

Copy link

mention-bot commented May 25, 2017

@fengmk2, thanks for your PR! By analyzing the history of the files in this pull request, we identified @dead-horse, @popomore and @ngot to be potential reviewers.

@fengmk2 fengmk2 requested a review from dead-horse May 25, 2017

@fengmk2

This comment has been minimized.

Copy link
Member

fengmk2 commented May 25, 2017

我得加测试用例。

@fengmk2 fengmk2 force-pushed the agent-disconnect branch from 8d80135 to b03ecdc May 25, 2017

@fengmk2 fengmk2 changed the title fix: listen disconnect event to let agent exit event master died by SIGKILL fix: agent should exit on disconnect event whatever master kill with SIGKILL May 25, 2017

@@ -23,12 +23,22 @@ agent.ready(() => {
agent.once('error', startErrorHandler);
function startErrorHandler(err) {
consoleLogger.error(err);
consoleLogger.error('[agent_worker] Agent Worker start error, exiting now!');
consoleLogger.error('[agent_worker] start error, exiting with code:1');

This comment has been minimized.

@atian25

atian25 May 25, 2017

Member

: 后空格,下面几个也是


function receiveSig(sig) {
debug('receive signal %s, exit with code 0, pid %s', sig, process.pid);

This comment has been minimized.

@atian25

atian25 May 25, 2017

Member

改为 arrow , 上面省去 bind?


process.once('disconnect', () => {
consoleLogger.info('[agent_worker] got disconnect event on child_process fork mode, exiting with code:0');
process.exit(0);

This comment has been minimized.

@dead-horse

dead-horse May 26, 2017

Member

讨论是 exit 1 的?

@codecov

This comment has been minimized.

Copy link

codecov bot commented May 27, 2017

Codecov Report

Merging #27 into master will decrease coverage by 1.4%.
The diff coverage is 82.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #27      +/-   ##
==========================================
- Coverage   96.59%   95.19%   -1.41%     
==========================================
  Files           7        7              
  Lines         294      333      +39     
==========================================
+ Hits          284      317      +33     
- Misses         10       16       +6
Impacted Files Coverage Δ
lib/app_worker.js 84.09% <60%> (-12.79%) ⬇️
lib/agent_worker.js 91.3% <81.81%> (-8.7%) ⬇️
lib/master.js 96.85% <90.9%> (+1.53%) ⬆️

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 873d779...e71d5dc. Read the comment docs.

@fengmk2 fengmk2 force-pushed the agent-disconnect branch from 42dc0f2 to ef40641 May 27, 2017

@fengmk2

This comment has been minimized.

Copy link
Member

fengmk2 commented May 27, 2017

加了一个 io loop 等 SIGTERM 事件发生,避免误判

ef40641

@fengmk2

This comment has been minimized.

Copy link
Member

fengmk2 commented May 27, 2017

终于搞定正常退出的完整提示日志了

image

@atian25

This comment has been minimized.

Copy link
Member

atian25 commented May 27, 2017

这个测试怎么没有触发 appveyor

// kill(15) default
process.once('SIGTERM', this.onSignal.bind(this, 'SIGTERM'));

process.once('exit', this.onExit.bind(this));

This comment has been minimized.

@fengmk2

@fengmk2 fengmk2 force-pushed the agent-disconnect branch from 1d6a571 to e71d5dc May 27, 2017

@@ -385,6 +419,17 @@ class Master extends EventEmitter {
}
require('cluster-reload')(this.options.workers);
}

close() {

This comment has been minimized.

@fengmk2

fengmk2 May 27, 2017

Member

实现优雅退出

process.once('exit', code => {
// istanbul can't cover here
// https://github.com/gotwarlost/istanbul/issues/567
this.killAgentWorker();

This comment has been minimized.

@dead-horse

dead-horse May 27, 2017

Member

原来之前是通过这里来 kill agent 的

This comment has been minimized.

@fengmk2

fengmk2 May 31, 2017

Member

对,写得比较挫

@dead-horse dead-horse merged commit 13fa854 into master May 28, 2017

4 of 6 checks passed

codecov/patch 82.85% of diff hit (target 96.59%)
Details
codecov/project 95.19% (-1.41%) compared to 873d779
Details
Node Security No known vulnerabilities found
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
security/snyk No new vulnerabilities
Details

@dead-horse dead-horse deleted the agent-disconnect branch May 28, 2017

@dead-horse

This comment has been minimized.

Copy link
Member

dead-horse commented May 28, 2017

1.6.4

process.once('SIGQUIT', receiveSig.bind(null, 'SIGQUIT'));
// kill(15) default
process.once('SIGTERM', receiveSig.bind(null, 'SIGTERM'));
onExit(code) {

This comment has been minimized.

@popomore

popomore May 28, 2017

Member

exit 不需要 close?

This comment has been minimized.

@fengmk2

fengmk2 May 31, 2017

Member

不需要了,进程马上就退出

This comment has been minimized.

@fengmk2

fengmk2 May 31, 2017

Member

onSignal => close => onExit(code)

@fengmk2

This comment has been minimized.

Copy link
Member

fengmk2 commented May 31, 2017

这个 fix 也证明,通过 child_process fork 出来的子进程,如果需要实现父进程挂了子进程也跟着挂,必须在子进程里面也加上相应的处理,才能实现,没办法只通过父进程来实现。

@fengmk2

This comment has been minimized.

Copy link
Member

fengmk2 commented May 31, 2017

哦,除非再起一个观察者进程,发现父进程挂了,然后去强制杀掉未退出的子进程,然后自杀。

@fengmk2

This comment has been minimized.

Copy link
Member

fengmk2 commented Jun 12, 2017

fengmk2 added a commit that referenced this pull request Jun 12, 2017

@fengmk2 fengmk2 referenced this pull request Jun 12, 2017

Merged

feat: use graceful-process to refactor app and agent worker #30

0 of 4 tasks complete

fengmk2 added a commit that referenced this pull request Jun 12, 2017

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