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

[protocol] connection: buffer messages until reconnect #5787

Merged
merged 2 commits into from
Sep 28, 2021

Conversation

geropl
Copy link
Member

@geropl geropl commented Sep 21, 2021

Description

This avoids Error("Connection got disposed") errors which gobble up to the frontend and block users. Also, it makes the frontend's websocket connection more resilient in general: Instead of dropping calls, those are buffered and re-issued once the connection is reestablished.

Related Issue(s)

Fixes #5042

How to test

Login to preview env

  1. go here and login
  2. note how you're re-directed on successful login with GitHub/GitLab

Start a workspace

  1. open devtools (console and network)
  2. start any workspace
  3. note how websocket connections work and open as expected

View build logs

  1. open a context that requires an image build
  2. wait until you see the log appearing
  3. terminate the websocket connection (by killing server)
  4. notice how the logs re-start scrolling automatically once server and the websocket connection is up-and-running again

Release Notes

improve websocket reconnection handling in the frontend
  • /werft with-clean-slate-deployment

@roboquat roboquat requested a review from laushinka September 21, 2021 13:03
@geropl geropl removed the request for review from laushinka September 21, 2021 13:13
@geropl geropl force-pushed the gpl/5042-conn-disposed branch 2 times, most recently from 5c97964 to fd985fc Compare September 21, 2021 14:29
This avoids Error("Connection got disposed") errors which gobble up to the frontend and block users.
@geropl geropl force-pushed the gpl/5042-conn-disposed branch from 2569049 to 4f14526 Compare September 22, 2021 14:52
@gitpod-io gitpod-io deleted a comment from roboquat Sep 22, 2021
@geropl geropl marked this pull request as ready for review September 22, 2021 15:55
@geropl geropl requested a review from akosyakov September 22, 2021 15:55
@geropl
Copy link
Member Author

geropl commented Sep 22, 2021

@akosyakov I'm not 100% sure that this is worth the effort (code complexity, potential fallout), but her it is 🤷 😉

@akosyakov
Copy link
Member

akosyakov commented Sep 23, 2021

/werft run

👍 started the job as gitpod-build-gpl-5042-conn-disposed.12

@akosyakov
Copy link
Member

/lgtm
@geropl I tested and everything seems to work.

/hold
see comment about logs, maybe they should be of debug level?

@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: 4acb2df23463a7bb5300d92035b349b5eefa590d

@geropl geropl force-pushed the gpl/5042-conn-disposed branch from ca1c7d4 to 05f8d75 Compare September 27, 2021 09:13
@roboquat roboquat removed the lgtm label Sep 27, 2021
@geropl
Copy link
Member Author

geropl commented Sep 27, 2021

see comment about logs, maybe they should be of debug level?

@akosyakov commented out log statements

@akosyakov
Copy link
Member

/lgtm

@roboquat roboquat added the lgtm label Sep 28, 2021
@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: 3a3881a7325f6905ed44f26ee3f7f7430ae27ecc

@akosyakov
Copy link
Member

/unhold

@geropl I think you need add approve label to merge it.

@geropl
Copy link
Member Author

geropl commented Sep 28, 2021

/approve

@roboquat
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akosyakov, geropl

Associated issue: #5042

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@roboquat roboquat merged commit cc29602 into main Sep 28, 2021
@roboquat roboquat deleted the gpl/5042-conn-disposed branch September 28, 2021 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[dashboard] Gracefully handle connection disposal
3 participants