- Notifications
You must be signed in to change notification settings - Fork948
feat: auto reconnect the terminal#18796
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
@code-asher I'm wondering if we can stop adding error messages to the terminal since we already have the alert. Wdyt?  |
Makes perfect sense to me! |
code-asher left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks promising! I tried it out and it seems to reconnect but then I was unable to see anything show up in the terminal. Is that the same for you? I can see messages being sent and received on the web socket, so it seems like we are just not printing to the terminal anymore. Maybe we have to rebind something? Or maybe the terminal is being recreated or something.
| @@ -259,9 +266,11 @@ const TerminalPage: FC = () => { | |||
| if (disposed) { | |||
| return; // Unmounted while we waited for the async call. | |||
| } | |||
| websocket = new WebSocket(url); | |||
| websocket = new WebsocketBuilder(url) | |||
| .withBackoff(new ConstantBackoff(1000)) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think we should switch this to exponential (ideally with some jitter) soon, if not in this PR. A fixed reconnect interval is a good way to DDoS yourself 😆
Ah actually it seems the data is a blob after reconnect instead of a array buffer? Does Also we should probably send the size on reconnect in case the user resized the terminal while offline. |
@code-asher fixed the issues you pointed out. I tested using the following steps:
Do you know if this is a good recipe for testing these changes? Let me know if you think I should test this differently. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It works! I wish their backoff implementation had jitter, but what can ya do. We can add our own backoff implementation later if necessary.
Put network on offline using the dev console (in network tab)
My experience has been that this setting does not affect existing connections (like web sockets). I tried just now to be sure (in Chromium) but the "trying to connect" message never appears and I can still use the terminal even when "offline".
What I have been doing is killing coderd, waiting for the reconnect message, then starting coderd again.
5a8a19b intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Changes:
useWithRetrybecause it is not necessary anymoreOther topics:
Closescoder/internal#659