Wait on `close` instead of `exit`. #47

Merged
merged 1 commit into from Sep 22, 2012

Projects

None yet

7 participants

Contributor
bigeasy commented Jul 3, 2012

If the version of Node.js is less than 0.7, wait on a "close" event from the ImageMagick process instead of "exit". As of 0.7, the exit "fires when the child actually exits, close fires when all stdio streams have closed."

See:

https://groups.google.com/forum/#!topic/nodejs/agsTbL-fco0/discussion

@bigeasy Your ternary condition will give a false negative once node reaches 1.0.

Contributor
bigeasy commented Jul 3, 2012

Yup. Will fix.

@bigeasy bigeasy Wait on `close` instead of `exit`.
If the version of Node.js is less than `0.7`, wait on a `"close"` event
from the ImageMagick process instead of `"exit"`. As of `0.7`, the
`exit` "fires when the child actually exits, `close` fires when all
stdio streams have closed."

See:

https://groups.google.com/forum/#!topic/nodejs/agsTbL-fco0/discussion
d356d0b
Contributor
bigeasy commented Jul 3, 2012

@TooTallNate Better?

dbogatz commented Jul 14, 2012

Maybe the same issue:
I noticed that "exit" sometimes get called before "data". So the callback gets called without any data collected in stdout.
I solved the issue simply by wrapping the exit-listener inside process.nextTick(function () {})

Having the same issue. When I run im.identify(path, function() {}), the code:

var v = stdout.split(/ +/),
  x = v[2].split(/x/);

fails; v[2] is null. This happens because exit is called before data. I don't know why, but the event handler from exit to close fixes it:

// in the exec2 function
child.addListener("close", function (code, signal) {

Going to try what @coyer suggested. @rsms can you update this please?

Side note, wrapping exit in process.nextTick doesn't always work for me, I'm having to set a timer to go off after about 50 milliseconds.

@viatropos Doing what @coyer suggested won't be enough as you're not guaranteed that close will have fired by then.

@bigeasy Are you sure close will always fire with both code & signal as arguments ? The node documentation doesn't seem to mention that anywhere?

@rsms Any chance , this gets included & pushed to npm? This is a really serious issue as its making appa crash and so blocks anybody using node-imagemagick from updating to node 0.8.x.

Contributor
bigeasy commented Aug 6, 2012

@glesperance Nope. Not sure at all. Let me ask around on the IRC and get back to you (with a patch if necessary).

Using https://github.com/aheckmann/gm, it solves all of these problems.

Contributor
bigeasy commented Aug 6, 2012

@isaacs says close is the new exit; it behaves as exit used to. Here's where close is emitted.

@bigeasy Awesome then.

@viatropos Thanks for the link. gm definitely looks awesome ; although I think I'll just make a fork of this package right now since I've got other deadlines.

nodejs/node-v0.x-archive#3846

It's said that close is fired when output buffers are closed. So it's right to wait on close instead of exit.

@rsms rsms merged commit 4e68d0e into rsms:master Sep 22, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment