fix: fix deno download retry logic #104
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| return binaryPath | ||
| } finally { | ||
| // Try closing and deleting the zip file in an case, error or not | ||
| await promisify(file.close.bind(file))() |
There was a problem hiding this comment.
Is there a chance that this not fire and we'll await indefinitely?
There was a problem hiding this comment.
Well I hope not. And that would be very unexpected behavior. If an error happens, then the callback(err) would be called -> reject.
eduardoboucas
previously approved these changes
Aug 26, 2022
eduardoboucas
approved these changes
Aug 26, 2022
Skn0tt
pushed a commit
to netlify/build
that referenced
this pull request
Apr 23, 2024
* fix: fix deno download retry logic * chore: try different windows deno binary * fix: cleanup on error, should also fix windows tests maybe * chore: deduplicate code * chore: close file * chore: fix rm * chore: fix node 12 * chore: fix review * Update test/downloader.ts
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes the retry logic that was introduced in #100.
The old logic would only retry if the fetch throws or the body would be empty, but not if the stream of the request body emits an error, which I think might be happening.
This PR now does two things:
Make the retry enclose the whole download method including fs-operations like unzipping, etc. This ensures now that if the stream emits an error we also retry in this case. If you think we should not add all fs-operations in the retry logic, then we can do that, but the diff gets bigger as I have to split up a bunch of methods.
If the fetch request does not return a 200 status-code we instantly error. Right now we do read the body and try to create the zip which fails, because the body most likely is not a valid zip.
I also added tests, which took the longest here :)
Not sure if the tests work on windows, but please start reviewing, even if windows is failing.