vscode: added error handling to download file streams #3116

Veetaha · 2020-02-11T21:33:47Z

As a followup for #3092
ts-nested-error is mine, it is just one file worth nothing, but let's us inspect original errors

Veetaha · 2020-02-11T21:38:58Z

Will rebase this on top of #3115, since I forsee merge conflicts

kjeremy · 2020-02-11T21:46:13Z

editors/code/src/installation/language_server.ts

                    `GitHub repository: ${err.message}`
                );

+                console.error(err);


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.

@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:

kjeremy · 2020-02-11T21:47:54Z

editors/code/src/installation/download_file.ts

 import { strict as assert } from "assert";
+import { NestedError } from "ts-nested-error";
+
+


I'd remove the extra line of white space.

kjeremy

Seems like a good idea to me! Left some nits. NOTE: I'm not terribly familiar with the TS.

Veetaha · 2020-02-11T21:53:13Z

@kjeremy, thank you for the review, I try to keep TS code as clean and easy to read as possible, so the nits are very welcome!

Veetaha · 2020-02-11T22:01:34Z

Removed whitespace, left console.error(), rebased the branch due to merge conflicts as mentioned in PR description.
Moved on(event, callback) calls for write-stream outside of pipe() call, since it returns the write-stream itself

matklad · 2020-02-12T12:49:23Z

Waiting on the resolution of #3092 (comment).

It seems like we should just use pipeline here. I think that gist is not necessary shows a problem: both the on('close') callback and the await pipeline should be run after close event, and I don't think there's any specifc ordering on them? Ie, i would imagine the on('close') thing might schedule a closure with SetTimeout(closure, 0), just to make sure it is not run on the stack which did the actual closing of the file .

matklad · 2020-02-13T09:45:26Z

editors/code/src/installation/download_file.ts

-        )
-    );
+    await pipeline(res.body, destFileStream).catch(DownloadFileError.rethrow("Piping file error"));
+    return new Promise<void>(resolve => {


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() }

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.

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

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'.

matklad · 2020-02-13T09:47:01Z

editors/code/src/installation/download_file.ts

+
+const pipeline = util.promisify(stream.pipeline);
+
+class DownloadFileError extends NestedError {}


We never catch this specific error, so I don't think it makes much sense to do error wrapping at all.

This is just that the name of the error (i.e. the name of the class reflectively) is displayed in the error message. Though we can infer where the Errror came from by the nested callstacks, so this is not that important...

Yeah, I would prefer to keep things simple by default, or rely on ubiquitous libraries (like rust's anynow) if there are big gains.

Sure, will simplify!

…city

Veetaha · 2020-02-13T20:22:26Z

@matklad, applied changes as per your comment

matklad · 2020-02-13T22:15:00Z

editors/code/src/installation/download_file.ts

    onProgress: (readBytes: number, totalBytes: number) => void
 ): Promise<void> {
-    const res = await fetch(url);
+    const res = await fetch(url).catch(NestedError.rethrow("Failed at initial fetch"));


I still don't think that using non-ubiquitous helper libraries is a good tradeoff.

Sorry, didn't understand you

Will remove NestedError then...

matklad · 2020-02-13T22:41:21Z

bors r+

3116: vscode: added error handling to download file streams r=matklad a=Veetaha As a followup for #3092 `ts-nested-error` is mine, it is just [one file worth nothing](https://github.com/Veetaha/ts-nested-error/blob/master/src/nested-error.ts), but let's us inspect original errors Co-authored-by: Veetaha <gerzoh1@gmail.com>

bors · 2020-02-13T22:48:25Z

Build succeeded

Rust (macos-latest)
Rust (ubuntu-latest)
Rust (windows-latest)
TypeScript

Veetaha requested review from kjeremy and matklad February 11, 2020 21:33

kjeremy reviewed Feb 11, 2020

View reviewed changes

kjeremy approved these changes Feb 11, 2020

View reviewed changes

vscode: added error handling to download file streams

36dc3ed

Veetaha force-pushed the feature/add-error-handling-to-download-file-streams branch from 759fe54 to 36dc3ed Compare February 11, 2020 21:59

Veetaha mentioned this pull request Feb 12, 2020

vscode: fix binary is not functional on windows #3092

Merged

vscode: switched to stream.pipeline with .on(close) workaround

a3febc1

matklad reviewed Feb 13, 2020

View reviewed changes

vscode: replaced DownloadFileError with NestedError itself for simpli…

da6ae3b

…city

Veetaha requested a review from matklad February 13, 2020 20:22

matklad reviewed Feb 13, 2020

View reviewed changes

vscode: removed nested errors as per matklad

574dc11

Veetaha force-pushed the feature/add-error-handling-to-download-file-streams branch from 4c63712 to 574dc11 Compare February 13, 2020 22:33

Veetaha requested a review from matklad February 13, 2020 22:34

bors bot merged commit 574dc11 into rust-lang:master Feb 13, 2020

Veetaha mentioned this pull request Feb 24, 2020

"Activating extension 'matklad.rust-analyzer' failed" #3280

Closed

Veetaha deleted the feature/add-error-handling-to-download-file-streams branch February 24, 2020 20:10

		import { strict as assert } from "assert";
		import { NestedError } from "ts-nested-error";


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

		class DownloadFileError extends NestedError {}

vscode: added error handling to download file streams #3116

vscode: added error handling to download file streams #3116

Uh oh!

Conversation

Veetaha commented Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Veetaha commented Feb 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjeremy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Veetaha commented Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Veetaha commented Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matklad commented Feb 12, 2020

Uh oh!

matklad Feb 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Veetaha commented Feb 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matklad commented Feb 13, 2020

Uh oh!

bors bot commented Feb 13, 2020

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Veetaha commented Feb 11, 2020 •

edited

Loading

kjeremy left a comment •

edited

Loading

Veetaha commented Feb 11, 2020 •

edited

Loading

Veetaha commented Feb 11, 2020 •

edited

Loading

matklad Feb 13, 2020 •

edited

Loading