fix: handle errors from file#createReadStream #615
Conversation
stephenplusplus
added
the
do not merge
googlebot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #615 +/- ##
==========================================
+ Coverage 97.92% 97.94% +0.01%
==========================================
Files 9 9
Lines 868 874 +6
Branches 99 99
==========================================
+ Hits 850 856 +6
Misses 9 9
Partials 9 9
Continue to review full report at Codecov.
|
stephenplusplus
force-pushed
the
spp--602
branch
from
8cb0406
to
4ec9daa
Compare
stephenplusplus
removed
the
do not merge
stephenplusplus
added
the
kokoro:force-run
kokoro-team
removed
the
kokoro:force-run
| rawResponseStream.on('end', onComplete).pipe(throughStream, { | ||
| end: false | ||
| }); | ||
| rawResponseStream.on('error', onComplete) |
There was a problem hiding this comment.
Is having 2 error listeners with the same callback intentional?
There was a problem hiding this comment.
The code above this actually re-assigns what rawResponseStream is. When we did that, it was to avoid duplicating the event handler assignments.
There was a problem hiding this comment.
Oh ok, sorry missed that part.
| end: false | ||
| }); | ||
| rawResponseStream.on('error', onComplete) | ||
| .on('end', onComplete) |
There was a problem hiding this comment.
Are there any scenarios where onComplete could fire multiple times? I sort of wonder if we should be using something like once to ensure that doesn't happen somehow.
There was a problem hiding this comment.
I 'spose anything is possible! I added new code for that, please take a look!
| const onComplete = (err: Error|null) => { | ||
| if (err) { | ||
| onCompleteCalled = true; | ||
| throughStream.destroy(err); |
There was a problem hiding this comment.
throughStream.destroy() seems to be once-ified automatically.
Fixes #602
To Dos
npm install stephenplusplus/nodejs-storage#spp--602