Skip to content
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.

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

Adopting dns result order changes with Node v18 update #189805

Closed
8 tasks done
deepak1556 opened this issue Aug 7, 2023 · 4 comments
Closed
8 tasks done

Adopting dns result order changes with Node v18 update #189805

deepak1556 opened this issue Aug 7, 2023 · 4 comments
Assignees
Labels
debt Code quality issues electron-25-update engineering VS Code - Build / issue tracking / etc. nodejs NodeJS support issues
Milestone

Comments

@deepak1556
Copy link
Collaborator

deepak1556 commented Aug 7, 2023

The change comes in via Electron 25 update #188268 which bumps our Node.js version from v16 -> v18. Node.js no longer sorts the DNS lookup results from the OS. This might break situations where one might expect to connect to a ipv4 localhost but OS resolves localhost to ipv6 address, which would result in ECONNREFUSED. Node.js has provided a couple of different settings to control this behavior

i) Process wide cli flag --dns-result-order=ipv4first|verbatim
ii) Per thread API in the dns module, dns.setDefaultResultOder
iii) Socket option autoSelectFamily to use happy eyeballs algorithm that will attempt to connect with ipv4 and ipv6 addresses until a connection is established.

I have adopted (i) in src/vs/server/node/extensionHostConnection.ts and src/vs/workbench/services/extensions/electron-sandbox/localProcessExtensionHost.ts to provide the old behavior in extension host spawned from both client and server. @alexdima let me know if we want to plan an action item to remove this workaround in future releases of VSCode.

This issue is created for callsites that are binding to 127.0.0.1 to verify if they are impacted by the Node.js change when connecting to these servers,

@deepak1556 deepak1556 added debt Code quality issues engineering VS Code - Build / issue tracking / etc. nodejs NodeJS support issues electron-25-update labels Aug 7, 2023
@deepak1556 deepak1556 added this to the August 2023 milestone Aug 7, 2023
@connor4312 connor4312 removed their assignment Aug 7, 2023
@alexr00 alexr00 removed their assignment Aug 7, 2023
@TylerLeonhardt
Copy link
Member

The auth extensions spin up servers that get open in the user's web browser. I verified the flows still work post-electron update.

@aeschli
Copy link
Contributor

aeschli commented Aug 17, 2023

@deepak1556 Is there some guidelines what to watch out for? My understanding is that when you listen to localhost, then you might now get end up with server on a v6 address (::1).
If you listen to 127.0.0.1 you get a server on the v4 address, as before.
That's why I think no changes are needed to https://github.com/microsoft/vscode/blob/main/extensions/vscode-test-resolver/src/extension.ts
Please let me know if there's more to it.

@aeschli aeschli removed their assignment Aug 17, 2023
@deepak1556
Copy link
Collaborator Author

My understanding is that when you listen to localhost, then you might now get end up with server on a v6 address (::1).
If you listen to 127.0.0.1 you get a server on the v4 address, as before.

Yup this is correct

What I would like to be verified is there are two servers listening on 127.0.0.1 in

proxyServer.listen(0, '127.0.0.1', () => {
and
proxyServer.listen(localPort, '127.0.0.1', () => {
. I am not sure if the client also connects on 127.0.0.1 or localhost and ensure that the connection is not broken with the Node v18 update.

@aeschli
Copy link
Contributor

aeschli commented Aug 17, 2023

I'm quite sure this still works as the test resolver is used for run our remote integration tests.

@github-actions github-actions bot locked and limited conversation to collaborators Oct 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues electron-25-update engineering VS Code - Build / issue tracking / etc. nodejs NodeJS support issues
Projects
None yet
Development

No branches or pull requests

7 participants