-
Notifications
You must be signed in to change notification settings - Fork 616
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
Enable error handling when streaming #647
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -79,7 +79,7 @@ module.exports = function (proto) { | |
* | ||
* @param {String} name | ||
* @param {Function} callback | ||
* @return {Object} gm | ||
* @return {null} | ||
*/ | ||
|
||
proto.write = function write (name, callback) { | ||
|
@@ -105,11 +105,14 @@ module.exports = function (proto) { | |
/** | ||
* Execute the command and return stdin and stderr | ||
* ReadableStreams providing the image data. | ||
* If a callback is passed, it is invoked immediately. To get process | ||
* errors or the exit status, listen for the "error" event on the | ||
* ChildProcess which is passed to the callback. | ||
* If no callback is passed, a "through" stream will be returned, | ||
* and stdout will be piped through, otherwise the error will be passed. | ||
* | ||
* @param {String} format (optional) | ||
* @param {Function} callback (optional) | ||
* @param {Function} callback (optional), signature (err, stdout, stderr, cmd, proc) | ||
* @return {Stream} | ||
*/ | ||
|
||
|
@@ -123,9 +126,12 @@ module.exports = function (proto) { | |
|
||
if ("function" !== typeof callback) { | ||
throughStream = new PassThrough(); | ||
callback = function (err, stdout, stderr) { | ||
callback = function (err, stdout, stderr, cmd, proc) { | ||
if (err) throughStream.emit('error', err); | ||
else stdout.pipe(throughStream); | ||
proc.on('error', function (err) { | ||
throughStream.emit('error', err); | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not strictly necessary, but seems useful. ^^ |
||
} | ||
} | ||
|
||
|
@@ -196,8 +202,8 @@ module.exports = function (proto) { | |
* @param {Array} args | ||
* @param {ReadableStream} stream | ||
* @param {Boolean} shouldBuffer | ||
* @param {Function} callback, signature (err, stdout, stderr) -> * | ||
* @return {Object} gm | ||
* @param {Function} callback, signature (err, stdout, stderr, cmd, proc) -> * | ||
* @return {null} | ||
* @TODO refactor this mess | ||
*/ | ||
|
||
|
@@ -226,14 +232,6 @@ module.exports = function (proto) { | |
return cb(e); | ||
} | ||
proc.stdin.once('error', cb); | ||
|
||
proc.on('error', function(err){ | ||
if (err.code === 'ENOENT') { | ||
cb(new Error('Could not execute GraphicsMagick/ImageMagick: '+cmd+" this most likely means the gm/convert binaries can't be found")); | ||
} else { | ||
cb(err); | ||
} | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved this block for clarity. It is only effective when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. Just, to explain for the next reviewer:
So, when |
||
|
||
if (timeout) { | ||
timeoutId = setTimeout(function(){ | ||
|
@@ -296,30 +294,38 @@ module.exports = function (proto) { | |
stderr += data; | ||
}); | ||
|
||
proc.on('error', function(err){ | ||
if (err.code === 'ENOENT') { | ||
cb(new Error('Could not execute GraphicsMagick/ImageMagick: '+cmd+" this most likely means the gm/convert binaries can't be found")); | ||
} else { | ||
cb(err); | ||
} | ||
}); | ||
|
||
proc.on('close', onExit = function (code, signal) { | ||
if (code !== 0 || signal !== null) { | ||
err = new Error('Command failed: ' + stderr); | ||
err.code = code; | ||
err.signal = signal; | ||
}; | ||
cb(err, stdout, stderr, cmd); | ||
cb(err, stdout, stderr); | ||
stdout = stderr = onOut = onErr = onExit = null; | ||
}); | ||
} else { | ||
cb(null, proc.stdout, proc.stderr, cmd); | ||
cb(null, proc.stdout, proc.stderr); | ||
} | ||
|
||
return self; | ||
|
||
function cb (err, stdout, stderr, cmd) { | ||
function cb (err, stdout, stderr) { | ||
if (cb.called) return; | ||
if (timeoutId) clearTimeout(timeoutId); | ||
cb.called = 1; | ||
if (args[0] !== 'identify' && bin !== 'identify') { | ||
self._in = []; | ||
self._out = []; | ||
self._in = []; | ||
self._out = []; | ||
} | ||
callback.call(self, err, stdout, stderr, cmd); | ||
callback.call(self, err, stdout, stderr, cmd, proc); | ||
} | ||
|
||
function dispose (msg) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No change in behavior. This is a documentation fix.