Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 20 additions & 13 deletions editors/code/src/installation/download_file.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
import fetch from "node-fetch";
import * as fs from "fs";
import * as stream from "stream";
import * as util from "util";
import { strict as assert } from "assert";

const pipeline = util.promisify(stream.pipeline);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd remove the extra line of white space.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do!

/**
* Downloads file from `url` and stores it at `destFilePath` with `destFilePermissions`.
* `onProgress` callback is called on recieveing each chunk of bytes
Expand All @@ -20,25 +24,28 @@ export async function downloadFile(
console.log("Error", res.status, "while downloading file from", url);
console.dir({ body: await res.text(), headers: res.headers }, { depth: 3 });

throw new Error(`Got response ${res.status} when trying to download a file`);
throw new Error(`Got response ${res.status} when trying to download a file.`);
}

const totalBytes = Number(res.headers.get('content-length'));
assert(!Number.isNaN(totalBytes), "Sanity check of content-length protocol");

console.log("Downloading file of", totalBytes, "bytes size from", url, "to", destFilePath);

let readBytes = 0;
res.body.on("data", (chunk: Buffer) => {
readBytes += chunk.length;
onProgress(readBytes, totalBytes);
});

console.log("Downloading file of", totalBytes, "bytes size from", url, "to", destFilePath);
const destFileStream = fs.createWriteStream(destFilePath, { mode: destFilePermissions });

await pipeline(res.body, destFileStream);
return new Promise<void>(resolve => {
Copy link
Contributor

@matklad matklad Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need a Promise here at all? I would imagine that the following would work (with some exception safety added just in case):

try {
    await pipeline(res.body, destFileStream);
    return;
} finally {
    destFileStream.destory()
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately it doesn't work on my laptop on windows ;( We still need to await the close event... Please see that discussion you originally referred to.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry, I've missed the last bit of the discussion :( Yeah, I guess the current code is what we should do then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, regarding destroy() in finally, this is a bit convoluted...
The streams do get automatically disposed on "error" event. See documentation

stream.pipeline() will call stream.destroy(err) on all streams except:

Readable streams which have emitted 'end' or 'close'.
Writable streams which have emitted 'finish' or 'close'.

destFileStream.on("close", resolve);
destFileStream.destroy();

return new Promise<void>((resolve, reject) => res.body
.on("data", (chunk: Buffer) => {
readBytes += chunk.length;
onProgress(readBytes, totalBytes);
})
.on("error", reject)
.pipe(fs
.createWriteStream(destFilePath, { mode: destFilePermissions })
.on("close", resolve)
)
);
// Details on workaround: https://github.com/rust-analyzer/rust-analyzer/pull/3092#discussion_r378191131
// Issue at nodejs repo: https://github.com/nodejs/node/issues/31776
});
}
2 changes: 2 additions & 0 deletions editors/code/src/installation/language_server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@ export async function ensureLanguageServerBinary(
`GitHub repository: ${err.message}`
);

console.error(err);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% sure we need to write the error out since the message is shown in vscode but I don't feel strongly about it. If you think it makes sense then that's fine by me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kjeremy err.message is as less verbose as it can, err.stack and console.error(err) are for the better debugging experience.
Here is the comparison:
image


dns.resolve('example.com').then(
addrs => console.log("DNS resolution for example.com was successful", addrs),
err => {
Expand Down