FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix: wrap socket in proxy before passing to vscode by jsjoeio · Pull Request #4840 · coder/code-server · GitHub

  1. coder
  2. code-server

Repository navigation

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

Merged
jsjoeio merged 7 commits into
mainfrom
jsjoeio-fix-cannot-reconnect
Feb 15, 2022
ConversationCommits (7)ChecksFiles changed
Merged

fix: wrap socket in proxy before passing to vscode#4840
jsjoeio merged 7 commits into
mainfrom
jsjoeio-fix-cannot-reconnect

Conversation

jsjoeio commented Feb 8, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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.

Fixes #4693

jsjoeio self-assigned this Feb 8, 2022
jsjoeio temporarily deployed to CI February 8, 2022 22:06 Inactive

github-actions Bot commented Feb 8, 2022 •
edited
Loading

Copy link
Copy Markdown

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

codecov Bot commented Feb 8, 2022 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

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

@@            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 temporarily deployed to CI February 8, 2022 22:19 Inactive
jsjoeio temporarily deployed to CI February 8, 2022 22:32 Inactive
jsjoeio temporarily deployed to CI February 9, 2022 21:45 Inactive
jsjoeio temporarily deployed to CI February 9, 2022 22:54 Inactive
jsjoeio mentioned this pull request Feb 10, 2022
Closed
jsjoeio temporarily deployed to CI February 10, 2022 21:30 Inactive
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 force-pushed the jsjoeio-fix-cannot-reconnect branch from 6aafd04 to af2792f Compare February 10, 2022 23:44
jsjoeio temporarily deployed to CI February 10, 2022 23:45 Inactive
jsjoeio force-pushed the jsjoeio-fix-cannot-reconnect branch from af2792f to 3580cf7 Compare February 10, 2022 23:45
jsjoeio temporarily deployed to CI February 10, 2022 23:45 Inactive
jsjoeio commented Feb 10, 2022
View reviewed changes
Comment thread package.json
Comment thread src/node/routes/vscode.ts
Comment thread test/e2e/baseFixture.ts Outdated
Comment thread test/e2e/extensions.test.ts
Comment thread test/e2e/models/CodeServer.ts
jsjoeio marked this pull request as ready for review February 10, 2022 23:49
jsjoeio requested a review from a team February 10, 2022 23:49
jsjoeio temporarily deployed to npm February 10, 2022 23:54 Inactive
code-asher approved these changes Feb 11, 2022
View reviewed changes

code-asher left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

🎉 🎉 🎉

Comment thread test/e2e/models/CodeServer.ts Outdated

jsjoeio commented Feb 11, 2022

Copy link
Copy Markdown
Contributor Author

Re-run e2e tests after merging: #4857

jsjoeio temporarily deployed to CI February 11, 2022 19:57 Inactive
jsjoeio temporarily deployed to CI February 11, 2022 19:57 Inactive

jsjoeio commented Feb 11, 2022

Copy link
Copy Markdown
Contributor Author

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

Need to add back in

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

jsjoeio commented Feb 14, 2022

Copy link
Copy Markdown
Contributor Author

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

jsjoeio force-pushed the jsjoeio-fix-cannot-reconnect branch from 94149b5 to 6049dd3 Compare February 15, 2022 21:20
jsjoeio temporarily deployed to CI February 15, 2022 21:20 Inactive
jsjoeio temporarily deployed to npm February 15, 2022 21:36 Inactive
jsjoeio merged commit e3e9f05 into main Feb 15, 2022
jsjoeio deleted the jsjoeio-fix-cannot-reconnect branch February 15, 2022 21:51
ghost mentioned this pull request Feb 15, 2022
Closed

ericzhucode commented Feb 16, 2022

Copy link
Copy Markdown

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

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 commented Feb 16, 2022

Copy link
Copy Markdown
Contributor Author

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

ericzhucode commented Feb 18, 2022

Copy link
Copy Markdown

@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 commented Feb 18, 2022

Copy link
Copy Markdown
Contributor Author

@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
fix: wrap socket in proxy before passing to vscode (coder#4840)
6821727 
* 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

This branch was previously deployed

2 inactive deployments
npm — 6049dd3b Deployed Feb 15, 2022 by jsjoeio
CI — 6049dd3b Deployed Feb 15, 2022 by jsjoeio
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

code-asher code-asher approved these changes

Assignees

jsjoeio

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Cannot reconnect. Please reload the window.

3 participants

Back | FazBrowse Home | New Git URL