Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

Install script should not write partially downloaded binaries #1912

Merged
merged 1 commit into from
Mar 10, 2018

Conversation

xzyfer
Copy link
Contributor

@xzyfer xzyfer commented Feb 22, 2017

Currently the binary download is streamed to disk once a 200 response
has been recieved. When an error occurs during the download a partially
downloaded binary is left on disk. Subsequent installs see the binary
and bail out of re-downloading it. Worse yet those subsequent installs
move the binary into the global cache so even removing node_modules
will not remove the broken binary.

With this patch the binary is only flushed to disk once it has been
fully downloaded.

Fixes #1882
Fixes #1888

@xzyfer
Copy link
Contributor Author

xzyfer commented Feb 22, 2017

Interesting the failure results in process.dlopen error message. This could be the root cause for so many of our previous issues.

module.js:598
  return process.dlopen(module, path._makeLong(filename));

@saper
Copy link
Member

saper commented Jul 10, 2017

What about going a bit further - writing to a temporary file and renaming only when completed?

@xzyfer
Copy link
Contributor Author

xzyfer commented Jul 11, 2017

That's certainly another alternative if we keep with the current approach of flushing to disk as data is received.

I've instead decided to go only flushing when the download is completed. Since the binary is only ~2MB I'm ok with leaving it memory until complete.

Currently the binary download is streamed to disk once a 200 response
has been recieved. When an error occurs during the download a partially
downloaded binary is left on disk. Subsequent installs see the binary
and bail out of re-downloading it. Worse yet those subsequent installs
move the binary into the global cache so even removing node_modules
will not remove the broken binary.

With this patch the binary is only flushed to disk once it has been
fully downloaded.

Fixes sass#1882
Fixes sass#1888
@xzyfer xzyfer self-assigned this Mar 10, 2018
@xzyfer xzyfer merged commit 3898350 into sass:master Mar 10, 2018
@xzyfer xzyfer deleted the fix-partial-download branch March 10, 2018 08:58
xzyfer added a commit that referenced this pull request Mar 11, 2018
Due to #1912 we were writing binaries to disk with the encoding.
We need to do some additional juggling when dealing with binary
data.
xzyfer added a commit that referenced this pull request Mar 11, 2018
Due to #1912 we were writing binaries to disk with the encoding.
We need to do some additional juggling when dealing with binary
data.
xzyfer added a commit that referenced this pull request Mar 11, 2018
Due to #1912 we were writing binaries to disk with the encoding.
We need to do some additional juggling when dealing with binary
data.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants