feat: [M3-8722] - Improve weblish retry behavior #11067
Merged
Conversation
This file contains 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
* automatically reconnect weblish sessions that have disconnected * … unless it fails rapidly (more than 3 times in a 1 minute window) * … in which case show an error screen with as much information as we can provide * … where the error screen includes a button to try again. * focus the terminal element when starting (which is generally helpful but also prevents _losing_ focus after a reconnect) Previously, a failure would often just result in a spinning circle forever.
plisiecki1
requested review from
hana-akamai and
cpathipa
and removed request for
a team
|
Coverage Report: ❌ |
hana-akamai
reviewed
Oct 9, 2024
…Error`. Break the `formatError` logic into two separate function that compute the two parts of the output, and then the main function just appends them. Adopt the preferred `const x = () =>` syntax in lieu of `function x()` syntax.
…s as its own, separate interface.
…iable as suggested in code review.
…d of `class` for `RetryLimiter`.
hana-akamai
reviewed
Oct 9, 2024
* Remove comments for obvious fields * `actionButton` => `actionButtonProps` * Combine `try` cases when parsing errors
hana-akamai
reviewed
Oct 16, 2024
* Add spaces around `=>` types. * Remove unnecessary `Optional Action Button` comment. * Refactor error handling using new `ParsePotentialLishErrorString` and `LishErrorInterface`. This centralizes all of the code to check structure and type of errors in one place. This may also prove using for future improvements to graphical lish. * Flatten websocket `close` handler logic from if-if-else-else structure to if-return-if-return-return.
hana-akamai
reviewed
Oct 16, 2024
hana-akamai
reviewed
Oct 17, 2024
* Remove second error format: It turns out that it will not be too difficult to make all of the errors follow the same basic format, so we don't need two separate parsers. Yay!
hana-akamai
approved these changes
Oct 22, 2024
cpathipa
approved these changes
Oct 23, 2024
cpathipa
added
Approved
coliu-akamai
approved these changes
Oct 23, 2024
|
|
||
| let parsed = null; | ||
| try { | ||
| parsed = JSON.parse(s); |
There was a problem hiding this comment.
any chance we know what the type/shape of parsed is, instead of using any?
There was a problem hiding this comment.
Until we do the checks following the quoted code, we don't know whether the incoming string actually conforms to the expected shape yet. The whole purpose of this function is to check the shape, and then if it is indeed in the expected shape, return the information that is useful to the code handling retries and errors. If we added an explicit type, it would just be used to cast away the "any" type once the shape is confirmed, then the code would immediately extract a few fields, and then discard that type. I don't think the additional overhead would add to the clarity or safety of the code.
Cloud Manager E2E
|
||||||||||||||||||||||||||||
| Project |
Cloud Manager E2E
|
| Branch Review |
develop
|
| Run status |
|
| Run duration | 25m 04s |
| Commit |
|
| Committer | plisiecki1 |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
2
|
Skipped
|
0
|
Passing
|
438
|
| View all changes introduced in this branch ↗︎ | |
This was referenced Oct 23, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description 📝
Previously, a failure would often just result in a spinning circle forever.
Changes 🔄
List any change relevant to the reviewer.
Lish.tsxwhich may also be useful for similar enhancements to graphical lish (glish) in the future.socketto be mutable and nullable so it can be updated when reconnecting.origSocketlocal variable is used to ensure that “stale” events are properly ignored.closehandler, which also eliminates the potential for console output that too closely matches an "expired" error being interpreted as an error and breaking the session immediately.Target release date 🗓️
TBD.
Preview 📷
How to test 🧪
Prerequisites
Running linode(s).
Reproduction steps
Launch a lish session for a machine, type
^a d kill <enter>. The session will exit and show a spinning circle.Verification steps
^a d kill <enter>. The session will reconnect.As an Author I have considered 🤔
Check all that apply