Skip to content

Remove stream.pipeline(); on("close", resolve) workaround #3167

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
Veetaha opened this issue Feb 16, 2020 · 13 comments
Closed

Remove stream.pipeline(); on("close", resolve) workaround #3167

Veetaha opened this issue Feb 16, 2020 · 13 comments
Labels
A-vscode vscode plugin issues S-unactionable Issue requires feedback, design decisions or is blocked on other work

Comments

@Veetaha
Copy link
Contributor

Veetaha commented Feb 16, 2020

Since the following bug in nodejs fs core module was fixed nodejs/node#31776 we need to get rid of the following workaround as per nodejs/node#31776 (comment) https://github.com/rust-analyzer/rust-analyzer/blob/77d27c67c1dadce4abe7a6a97c0b73eaa3706e60/editors/code/src/installation/download_file.ts#L44-L50

ACHTUNG! Wait until vscode officially migrates to relevant nodejs version where this is fixed, update our vscode version requirements and only then remove the workaround.
Current version 1.42 of vscode uses nodejs 12.4
image
@Veetaha Veetaha changed the title Remove stream stream.pipeline(); on("close", resolve) workaround Remove stream.pipeline(); on("close", resolve) workaround Feb 16, 2020
@Veetaha
Copy link
Contributor Author

Veetaha commented Jun 19, 2020

History of the bug and its detection: #3092 (comment)

We probably need to wait until vscode adopts nodejs 13.x+ version.

@lnicola lnicola added the A-vscode vscode plugin issues label Jul 15, 2020
@hjfreyer
Copy link

I think this workaround may have caused a bug for me.

Steps to reproduce

  1. Use code-server in the browser.
  2. Install rust-analyzer extension.
  3. Reload the webpage.
  4. Notice the "Language server version ... for rust-analyzer is not installed" message.
  5. Click "Download now"

Expected result

The file downloads and the installation continues.

Actual result

Progress freezes after "Downloading rust-analyzer server 100%".

Analysis

I don't really get why, but the code gets stuck on this await block:

https://github.com/Veetaha/rust-analyzer/blob/3c749b6224282e18db264b1e2e6f19f14b5c0a26/editors/code/src/net.ts#L124-L130

If I comment it out, it appears to work. Hopefully it's not subtly breaking anything!

Version Info

code-server version: 3.4.1 48f7c2724827e526eeaa6c2c151c520f48a61259
nodejs version: v10.21.0

@Veetaha
Copy link
Contributor Author

Veetaha commented Aug 21, 2020

That's strange, you mentioned nodejs 10, but current vscode uses nodejs 12. Is it possible to upgrade the runtime version for you?

@hjfreyer
Copy link

Oops, I was wrong about the nodejs version. My system-wide nodejs was 10, but digging further I found that code-server bundles its own node that's currently v14.4.0. However, I can't for the life of me figure out why it's v14, not v12. The patchnotes don't mention an upgrade to v14...

@Veetaha
Copy link
Contributor Author

Veetaha commented Aug 21, 2020

Yeah I suppose that since the inital bug was fixed in v13, this workaround might actually harm on node v13+
Since vscode still uses v12 no one has tried running this on v13+
I guess we might just add a small if to check for the current node version

@hjfreyer
Copy link

Aha, turns out there WAS already a bug tracking this: coder/code-server#1810

@hjfreyer hjfreyer mentioned this issue Aug 21, 2020
Closed
@Veetaha Veetaha mentioned this issue Aug 21, 2020
Merged
bors bot added a commit that referenced this issue Aug 24, 2020
@bors @Veetaha
Merge #5841
d7f4711 
5841: Gate stream.pipeline workaround on fixed versions of node r=matklad a=Veetaha

Fixes the symptom of coder/code-server#1810
Original report here: #3167 (comment)
Thanks to @hjfreyer for precise investigation :D

Co-authored-by: Veetaha <[email protected]>
@lnicola lnicola added the S-unactionable Issue requires feedback, design decisions or is blocked on other work label Jan 22, 2021
@Azorlogh
Copy link
Contributor

VSCode is now on Node v14, is this still needed?

@lnicola
Copy link
Member

lnicola commented Aug 26, 2021

@Azorlogh maybe not, want to file a PR?

@Azorlogh
Copy link
Contributor

Sure!

@Veetaha
Copy link
Contributor Author

Veetaha commented Aug 27, 2021

Well, it depends on what versions of vscode we support. It's specified in package.json here:
https://github.com/rust-analyzer/rust-analyzer/blob/8f683e911c2a9d0d287dfcc354ca07b8beb63725/editors/code/package.json#L24

If this version and the following ones already use node14, then we are good to go!

@Azorlogh
Copy link
Contributor

VSCode made the switch to Node 14 in April in version 1.56.

@Azorlogh Azorlogh mentioned this issue Aug 27, 2021
Merged
bors bot added a commit that referenced this issue Aug 27, 2021
@bors @Azorlogh
Merge #10053
4ad0a1b 
10053: Remove old workaround in vscode extension r=lnicola a=Azorlogh

See #3167.

Co-authored-by: = <[email protected]>
@Veetaha
Copy link
Contributor Author

Veetaha commented Aug 27, 2021 •
edited
Loading

Thanks for tackling this! I wonder how one could find such an old and S-unactionable issue and close it =)

@Veetaha Veetaha closed this as completed Aug 27, 2021
@Azorlogh
Copy link
Contributor

Haha I've been peeking into the codebase lately.

@mtsmfm mtsmfm mentioned this issue Aug 28, 2021
Merged
bors bot added a commit that referenced this issue Aug 28, 2021
@bors @mtsmfm
Merge #10062
2e3322d 
10062: Set esbuild target as node14 r=matklad a=mtsmfm

ref: #10061

Currently, target version is not specified so it's esnext.

https://esbuild.github.io/api/#target

VSCode uses node 14 since version 1.56.

#3167 (comment)

Co-authored-by: Fumiaki MATSUSHIMA <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-vscode vscode plugin issues S-unactionable Issue requires feedback, design decisions or is blocked on other work
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@hjfreyer @lnicola @Azorlogh @Veetaha