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

[WIP]Reconnection and Session improvements for Shell App #479

Closed
wants to merge 21 commits into from

Conversation

samirmansour
Copy link
Contributor

This is a new branch with the same functionality as terminal-decouple branch, but it is refactored to work with the new changes made to master. It is also an improvement on the reconnection experience for the user.

@samirmansour samirmansour changed the title Reconnection and Session improvements for Shell App *WIP* [WIP]Reconnection and Session improvements for Shell App Apr 20, 2020
…make sure that the websocket connection is alive. If dead it reconnects without a refresh.
apps/shell/app.js Outdated Show resolved Hide resolved
@johrstrom
Copy link
Contributor

This PR didn't seem to redirect from /pun/dev/shell correctly. It seems that /pun/dev/shell redirects to /pun/dev/shell/ssh which returns a 404 with a header Content-Security-Policy | default-src 'none' so that the next request (for the favicon.ico) doesn't meet the content security policy.

Though c3c80d4 seems to fix it, it may cause other issues or need more work so I thought I'd still document the behaviour.

I've narrowed it down to trailing slashes. /pun/dev/shell/ works where /pun/dev/shell
doesn't. To replicate you'd simply need to manually edit the URL in the browser after resetting back to c5d3d95 and rebuilding.

Here's a screenshot.
image

@ericfranz
Copy link
Contributor

The favicon issue is a preexisting issue. The url is wrong. It is to /favicon.ico not /public/favicon.ico.

@samirmansour samirmansour requested a review from ericfranz June 17, 2020 15:23
@msquee
Copy link
Contributor

msquee commented Jun 17, 2020

@samirmansour There's a bug that causes an error to be thrown every other page refresh.

Here's the error:
image

I tracked down where the error is being thrown: https://github.com/microsoft/node-pty/pull/285/files#diff-32a8618c58cb849e9b5535ba19ffacc5R369-R375

Here's a thread I found with people having the same issue: microsoft/node-pty#220 (comment)

Are you able to reproduce this?

* term was being killed instead of paused when the WS disconnected

* resize term immediately after resuming term session
apps/shell/app.js Outdated Show resolved Hide resolved
@samirmansour samirmansour requested a review from ericfranz July 16, 2020 16:14
@msquee
Copy link
Contributor

msquee commented Jul 17, 2020

@samirmansour We should use Close Code 1001 instead of 4000. https://tools.ietf.org/html/rfc6455#section-7.4.1

From section 7.4.1 in RFC6455:
1001 indicates that an endpoint is "going away", such as a server
going down or a browser having navigated away from a page.

@samirmansour
Copy link
Contributor Author

@samirmansour We should use Close Code 1001 instead of 4000. https://tools.ietf.org/html/rfc6455#section-7.4.1

From section 7.4.1 in RFC6455:
1001 indicates that an endpoint is "going away", such as a server
going down or a browser having navigated away from a page.

Discussed on Slack. Using code 4000 to signify that a user is closing the browser or navigating away to another site. If default codes are used then code 1001 will be sent by default for disconnection via wifi or if a user navigates away. WIFI loss should not terminate the terminal session, but user navigating away should.

@ericfranz ericfranz added this to the OOD2.0 milestone Aug 4, 2020
@ericfranz ericfranz modified the milestones: OOD2.0, OOD2.1 Oct 20, 2020
@ericfranz ericfranz modified the milestones: OOD2.1, OOD2.0 Feb 27, 2021
@johrstrom
Copy link
Contributor

This has merge conflicts at this point. What's more though, I think, is that I'm looking for a much simpler just attempt to reconnect a few times.

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.

5 participants