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 appveyor build #24

Merged
merged 10 commits into from
Oct 29, 2016
29 changes: 29 additions & 0 deletions appveyor.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# Test against this version of Node.js
environment:
matrix:
- nodejs_version: "4"
- nodejs_version: "5"
- nodejs_version: "6"

platform:
- x86
- x64

# Install scripts. (runs after repo cloning)
install:
# Get the latest stable version of Node.js or io.js
- ps: Install-Product node $env:nodejs_version $env:platform
# install modules
- npm install

# Post-install test scripts.
test_script:
# Output useful info for debugging.
- node --version
- npm --version
# run tests
- npm run coverage

# Don't actually build.
build: off

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
},
"homepage": "https://github.com/dwyl/terminate",
"dependencies": {
"ps-tree": "1.0.1"
"ps-tree": "^1.1.0"
},
"devDependencies": {
"chalk": "^1.1.1",
Expand Down
26 changes: 17 additions & 9 deletions terminate.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
var cp = require('child_process');
var psTree = require('ps-tree'); // see: http://git.io/jBHZ

/**
Expand All @@ -17,13 +16,22 @@ module.exports = function terminate(pid, callback) {
throw new Error("No pid supplied to Terminate!")
}
psTree(pid, function (err, children) {
cp.spawn('kill', ['-9'].concat(children.map(function (p) { return p.PID })))
.on('exit', function() {
if(callback && typeof callback === 'function') {
callback(err, true);
} else { // do nothing
console.log(children.length + " Processes Terminated!");
}
});
children.forEach(function (child) {
try {
process.kill(parseInt(child.PID), 'SIGKILL');
} catch (error) {
// ignore
}
});
try {
process.kill(pid, 'SIGKILL');
} catch (error) {
// ignore
}
if(callback && typeof callback === 'function') {
callback(err, true);
} else { // do nothing
console.log(children.length + " Processes Terminated!");
}
});
};
2 changes: 2 additions & 0 deletions test/exec/child.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
// does nothing child process
console.log("Child process.id: "+process.pid);
console.log(" - - - - - - - - - - - - - - - - - - - - - - - ");
// keep process alive for one minute
setTimeout(function () {}, 60000);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jedwards1211 please clarify why this timeout is required and why it has to be for 60 sec. thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's not necessary, I got confused because the windows tests are still very timing-dependent, but I was surprised the tests were ever working because the child processes will exit once they finish those two console.logs. Using setTimeout keeps the process alive. The timeout definitely doesn't have to be for one minute though. It could just be a bit longer than the duration of the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as the timing of windows tests, I guess the best thing is to make the first test poll until all the child processes are gone, with a test timeout of 5 seconds or so (sometimes it takes more than 1 second for the child processes to go away).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree. poll. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you want to go ahead and merge this, or do you want me to implement polling?
Polling does have one drawback, I realized. It's unlikely but possible that a new process starts with the same pid as one that was killed, causing terminate to poll forever.
I have implemented polling in my crater-util package (ironically I ended up using that instead of this) and it works well enough for my integration tests though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. let's merge it and publish a new version. if we get a bug report we can go from there. 👍

2 changes: 1 addition & 1 deletion test/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ test(cyan('Spawn a Parent process which has a few Child Processes'), function (t
t.equal(children.length, 0, green("✓ No more active child processes (we killed them)"));
t.end();
})
},200); // give psTree time to kill the processes
},1000); // give psTree time to kill the processes
},10); // give the child process time to spawn
},200); // give the child process time to spawn
});
Expand Down