-
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?
Conversation
@@ -79,7 +79,7 @@ module.exports = function (proto) { | |||
* | |||
* @param {String} name | |||
* @param {Function} callback | |||
* @return {Object} gm | |||
* @return {null} |
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Not strictly necessary, but seems useful. ^^
} else { | ||
cb(err); | ||
} | ||
}); |
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.
Moved this block for clarity. It is only effective when bufferOutput
is true
. See discussion in #256.
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.
Makes sense. Just, to explain for the next reviewer:
- there's code to prevent the
callback
to be called more than once - when
!buffer
case calls the callback immediately
So, when !buffer
this error would be swallowed by the code that prevents the callback to be called more than once.
Same errors are appearing in CI. Again, I don't think they are related to this change. |
@aheckmann Could you take a look, please? Would be great to get this fixed for badges/shields#914. |
Hi @aheckmann , this is an important PR, otherwise the use of |
Hello, I bumped into this in one of my projects today and I more or less solved it by bypassing async function gmToBuffer (data) {
return new Promise((resolve, reject) => {
let chunks = [];
data._preprocess((err) => {
if (err) return reject(err);
data._spawn(data.args(), false, (err, stdout, stderr) => {
if (err) return reject(err);
stdout.on('data', (chunk) => chunks.push(chunk));
stdout.once('end', () => resolve(Buffer.concat(chunks)));
stderr.on('data', (msg) => {
const error_message = String(msg);
if (error_message.includes('@ warning/')) {
logger.error(error_message);
} else {
stdout.emit('error', new Error(error_message));
}
});
stdout.once('error', (err) => reject(err));
});
});
});
} And to be specific I wanted to still get the warning and not just silence them. |
See discussion of the problem in #256 (and a bit more in #548).
Adding
proc
to the.stream()
callback seems like the easiest way to allow the caller to handle this. Tested in a downstream package and it works, though it's not terribly convenient.When no callback is provided, emit an
error
event on the throughStream.Before and after this change, I have two failing tests, which appear to be unrelated.