-
-
Notifications
You must be signed in to change notification settings - Fork 13
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
Ignore unexpected EOF errors #1
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 |
---|---|---|
|
@@ -15,6 +15,15 @@ module.exports = function (res) { | |
unzip.statusMessage = res.statusMessage; | ||
unzip.socket = res.socket; | ||
|
||
unzip.once('error', function (err) { | ||
if (err.code === 'Z_BUF_ERROR') { | ||
res.emit('end'); | ||
return; | ||
} | ||
|
||
res.emit('error', err); | ||
}); | ||
|
||
res.on('close', function () { | ||
unzip.emit('close'); | ||
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. Btw, not sure you should use 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. Why is it better? I've yet to figure out the difference. Wouldn't 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. Ugh, seems there's no official way: http://stackoverflow.com/questions/19277094/how-to-close-a-readable-stream-before-end Node.js streams really are the worst... 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. I've encountered less inconsistencies when using
Yes. If it's a writable stream, 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. Alright 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. I'll do this in a major bump, just to be sure. |
||
}); | ||
|
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.
It looks like we should rather use
res.destroy()
here. https://nodejs.org/api/http.html#http_message_destroy_errorThere 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.
👍 , that's also inconsistent sometimes, not all streams implement
.destroy()
. nodejs/readable-stream#124.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.
Seems like it doesn't exist on
res
:TypeError: res.destroy is not a function
. Why can't they all behave like you think they should...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.
But but, documented. Ugh. Never mind then. Lol.