Fix #3697 Improve error handling when tarball fetcher receives bad hash #4240
Conversation
src/fetchers/tarball-fetcher.js
Outdated
|
|
||
| if (tarballCachePath) { | ||
| validateStream.pipe(fs.createWriteStream(tarballCachePath)).on('error', reject); | ||
| } |
There was a problem hiding this comment.
Won't that cause the tarballs to still be written on the disk even if the validation fails?
There was a problem hiding this comment.
Agree with @arcanis. I think what we need is keeping this in the old place and just deleting the file on validation failure before we reject the promise. This would also prevent going over the file two times.
There was a problem hiding this comment.
yup initial approach was only write when hash matches but realized there are other reasons that would cause failure. updated!
b9595e4 to
f4fa4ae
Compare
|
LGTM but last word is yours @arcanis |
| const tarballCachePath = this.getTarballCachePath(); | ||
|
|
||
| if (tarballMirrorPath && (await fsUtil.exists(tarballMirrorPath))) { | ||
| await fsUtil.unlink(tarballMirrorPath); |
There was a problem hiding this comment.
@arcanis just curious, why does the unlinking have to be synchronous here?
There was a problem hiding this comment.
One benefit I see is that uncaught exceptions will be logged to the console, and will possibly abort the execution in future Node versions. Apart from that, it shouldn't really matter, but if you do it I suggest you also add a comment to explain it's made on purpose, since it can also be seen as a typo.
… bad hash (yarnpkg#4240) * test * alter commit message * update snapshot * review feedback * move to unlink files in all failed blocks * lint * Update tarball-fetcher.js
Summary
Fix for #3697
finishblock of the stream when it checks for the matching hash.Test plan
Added a test case in fetchers
BEFORE
AFTER
*note hashes are the same in the screenshots bc I made the code block fail intentionally