Skip to content

fix: wrap socket in proxy before passing to vscode #4840

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

Merged
merged 7 commits into from
Feb 15, 2022

Conversation

jsjoeio
Copy link
Contributor

@jsjoeio jsjoeio commented Feb 8, 2022

Summary

This PR fixes a regression introduced in 4.0.1. Previously in 3.12.0, using the --cert flag allowed you to use code-server locally over HTTPS. The regression broke that functionality causing the ExtensionHost to repeatedly reconnect and fail. We fixed this by wrapping the socket in our SocketProxyProvider before passing back to VS Code. This is because TLSSockets cannot be passed to child processes.

Changes

  • modify e2e tests to accept args
  • add new e2e test to extension.test.ts with --cert flag
  • use req.ws instead of req.socket
  • wrap req.ws in SocketProxyProvider and pass in handleUpgrade

Context

We didn't have a test for this feature so the regression was introduced in 4.0.1. We've now added a new e2e test which uses the --cert flag and attempts to use an extension. This will only work if the ExtensionHost is working properly. Therefore, this will prevent new regressions.

After adding the new e2e test, we tested it against the 3.12.0 build and it worked as expected:

extension-proof.mp4

Here is that same test passing against code-server using a local dev build with the regression fixed:

4.0.2-proof.mp4

We also took extensive notes and whiteboarded the flow here to help us arrive at the solution.

cs-4693

Fixes #4693

@jsjoeio jsjoeio self-assigned this Feb 8, 2022
@jsjoeio jsjoeio temporarily deployed to CI February 8, 2022 22:06 Inactive
@github-actions
Copy link

github-actions bot commented Feb 8, 2022

✨ Coder.com for PR #4840 deployed! It will be updated on every commit.

@codecov
Copy link

codecov bot commented Feb 8, 2022

Codecov Report

Merging #4840 (6049dd3) into main (b26cce5) will increase coverage by 0.01%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4840      +/-   ##
==========================================
+ Coverage   69.72%   69.73%   +0.01%     
==========================================
  Files          29       29              
  Lines        1645     1649       +4     
  Branches      363      363              
==========================================
+ Hits         1147     1150       +3     
- Misses        424      425       +1     
  Partials       74       74              
Impacted Files Coverage Δ
src/node/routes/vscode.ts 80.30% <50.00%> (-0.35%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b26cce5...6049dd3. Read the comment docs.

@jsjoeio jsjoeio temporarily deployed to CI February 8, 2022 22:19 Inactive
@jsjoeio jsjoeio temporarily deployed to CI February 8, 2022 22:32 Inactive
@jsjoeio jsjoeio temporarily deployed to CI February 9, 2022 21:45 Inactive
@jsjoeio jsjoeio temporarily deployed to CI February 9, 2022 22:54 Inactive
@jsjoeio jsjoeio temporarily deployed to CI February 10, 2022 21:30 Inactive
@jsjoeio jsjoeio changed the title wip: add test for https and ws reconnect fix: wrap socket in proxy before passing to vscode Feb 10, 2022
@jsjoeio jsjoeio force-pushed the jsjoeio-fix-cannot-reconnect branch from 6aafd04 to af2792f Compare February 10, 2022 23:44
@jsjoeio jsjoeio temporarily deployed to CI February 10, 2022 23:45 Inactive
@jsjoeio jsjoeio force-pushed the jsjoeio-fix-cannot-reconnect branch from af2792f to 3580cf7 Compare February 10, 2022 23:45
@jsjoeio jsjoeio temporarily deployed to CI February 10, 2022 23:45 Inactive
@jsjoeio jsjoeio marked this pull request as ready for review February 10, 2022 23:49
@jsjoeio jsjoeio requested a review from a team February 10, 2022 23:49
@jsjoeio jsjoeio temporarily deployed to npm February 10, 2022 23:54 Inactive
Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

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

🎉 🎉 🎉

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Feb 11, 2022

Re-run e2e tests after merging: #4857

@jsjoeio jsjoeio temporarily deployed to CI February 11, 2022 19:57 Inactive
@jsjoeio jsjoeio temporarily deployed to CI February 11, 2022 19:57 Inactive
@jsjoeio
Copy link
Contributor Author

jsjoeio commented Feb 11, 2022

// TODO@jsjoeio add note why we do this for testing cert stuff
ignoreHTTPSErrors: true,

Need to add back in

@jsjoeio jsjoeio temporarily deployed to CI February 11, 2022 20:41 Inactive
@jsjoeio jsjoeio force-pushed the jsjoeio-fix-cannot-reconnect branch from db72d83 to 94149b5 Compare February 11, 2022 20:41
@jsjoeio jsjoeio temporarily deployed to CI February 11, 2022 20:41 Inactive
@jsjoeio
Copy link
Contributor Author

jsjoeio commented Feb 14, 2022

I believe the e2e tests are failing because we need to update VS Code (waiting on another PR)

@jsjoeio jsjoeio force-pushed the jsjoeio-fix-cannot-reconnect branch from 94149b5 to 6049dd3 Compare February 15, 2022 21:20
@jsjoeio jsjoeio temporarily deployed to CI February 15, 2022 21:20 Inactive
@jsjoeio jsjoeio temporarily deployed to npm February 15, 2022 21:36 Inactive
@jsjoeio jsjoeio merged commit e3e9f05 into main Feb 15, 2022
@jsjoeio jsjoeio deleted the jsjoeio-fix-cannot-reconnect branch February 15, 2022 21:51
@ericzhucode
Copy link

Hi team,
I have used the latest version of code-server and coder/vscode to build but it seems reconnection issue still exists
(including this fixup commit)
This is what I got in dev tools
image
May I know what's the situation on your side and does this matter?(since it's only info level but not error or warning)
Thank you!

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Feb 16, 2022

@ericzhucode what is the output when you run code-server --version?

@ericzhucode
Copy link

@ericzhucode what is the output when you run code-server --version?

Sorry I forgot to update this comment. After cherry-pick this commit and build code server again, it's working perfectly now and no disconnection occurred.
Thank you for your effort!

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Feb 18, 2022

@ericzhucode hooray!!! Really happy it's working. Thank you for confirming! 🎉

TinLe pushed a commit to TinLe/code-server that referenced this pull request Apr 23, 2022
* chore: add ipc hook to e2e script

* refactor: allow codeServerArgs in e2e tests

* feat: add --cert e2e extension test

* fix: wrap websocket in proxy

* fixup: remvoe ignoreHTTPSErrors

* fixup: make codeServerArgs readonly

* fixup! add back ignoreHTTPSErrors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot reconnect. Please reload the window.
3 participants