Automatic reconnect with Language Server. #9691

farmaazon · 2024-04-12T15:04:21Z

Pull Request Description

If the websocket is closed not by us, we automatically try to reconnect with it, and initialize the protocol again. Restoring state (execution contexts, attached visualizations) is not part of this PR.

It's a part of making IDE work after hibernation (or LS crash).

Important Notes

It required somewhat heavy refactoring:

I decided to use an existing implementation of reconnecting websocket. Replaced (later discovered by me) our implementation.
The LanguageServer class now handles both reconnecting and re-initializing - that make usage of it simpler (no more Promise<LanguageServer> - each method will just wait for (re)connection and initialization.
The stuff in net src's module was partially moved to shared's counterpart (with tests). Merged exponentialBackoff implementations, which also brought me to
Rewriting LS client, so it returns Result instead of throwing, what is closer our desired state, and allows us using exponentialBackoff method without any wrappers.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
~~[ ] Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.~~
All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- ~~[ ] Unit tests have been written where possible.~~
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

…/reconnecting

Frizi · 2024-04-16T11:52:31Z

app/gui2/shared/dataServer.ts

+/// <reference lib="DOM" />
+/* eslint-env browser */


How is this correct? This ordinarily will in node or JVM, so using DOM types seems invalid.

Frizi · 2024-04-16T11:54:28Z

app/gui2/shared/languageServer.ts

+    })
+  }
+
+  private scheduleInitializationAfterConnect() {


should this be scheduleConnectAndInitialize? The current name doesn't actually communicate that this does the "connect" part.

And this is valid: this function does not connect, because it does not make the reconnect (the websocket underneath does), only listens for "open" event to start initialization after.

Frizi · 2024-04-16T11:57:42Z

app/gui2/shared/languageServer.ts

-        throw new LsRpcError(error, method, params)
+        return Err(new LsRpcError(error, method, params))
+      } else {
+        return Err(new LsRpcError(`Unspecified error: ${error}`, method, params))


For errors that we actually don't expect to ever happen, I'd rather keep this as a throw error, treating it basically as a panic.

I think we expect some problems not being rpc responses (network errors, timeouts), and there is no documentation about those in open-rpc (at least I cannot find any), so I think it's save to catch all Errors. non-Error values I will rethrow.

Frizi · 2024-04-17T11:52:08Z

app/gui2/e2e/componentBrowser.spec.ts

@@ -254,6 +254,9 @@ test('Visualization preview: user visualization selection', async ({ page }) =>
  await input.fill('4')
  await expect(input).toHaveValue('4')
  await expect(locate.jsonVisualization(page)).toExist()
+  // Before picking table vis, we must make sure we wan't be hit by debounced visualization update
+  // during the process.
+  await page.waitForTimeout(250)


I would strongly prefer any other way to check for expected final state than by doing a timeout. The timeout times quickly add up to a significant total testing runtime, and also it has a high probability of false negatives.

Found another way; the preview presents now the expression, so we may wait until it shows the actually written code. I also fixed the bug where the previews always interpreted received object as a string.

Frizi · 2024-04-17T11:58:35Z

app/gui2/src/stores/suggestionDatabase/index.ts

+        groups.error.log('Cannot read component groups')
+        console.error('Continuing without gruops')


Those two logs could be merged, or one can be removed.

Frizi · 2024-04-17T12:05:43Z

app/gui2/shared/languageServer.ts

      console.error(`Unexpected LS connection error:`, error)
    })
+    transport.on('error', (error) => console.error('Language Server transport error:', error))


Not sure if this is significant, but this event handler is not being cleared in the onAbort.

I would actually recommend using handleObserve or similar method to register such handlers.

In this case the transport is "owned" by LanguageServer object and it's closed on dispose; it should not be used anywhere else (and if it is, or there are errors after closing, we ought to see them).

Frizi · 2024-04-17T12:11:08Z

app/gui2/shared/languageServerTypes.ts

@@ -344,6 +344,8 @@ export type Notifications = {
  'file/rootAdded': (param: {}) => void
  'file/rootRemoved': (param: {}) => void
  'refactoring/projectRenamed': (param: {}) => void
+  'transport/closed': () => void


Are those actual notifications related to language server protocol? If not, I would prefer those to be in a separate type definition. It can still be merged at the usage point as the ObservableV2 parameter.

farmaazon · 2024-04-18T07:57:07Z

Applied review and fix some tests. Fix the display of visualization preview, by the way.

Now I have problems with AI completion test, but I think the code is ready to re-review.

Frizi

Few last small nits.

Frizi · 2024-04-18T12:32:07Z

app/gui2/src/components/VisualizationSelector.vue

@@ -39,7 +39,7 @@ onMounted(() => setTimeout(() => rootNode.value?.querySelector('button')?.focus(
  <div
    ref="rootNode"
    class="VisualizationSelector"
-    @focusout="$event.relatedTarget == null && emit('hide')"
+    @focusout="$event.relatedTarget == null && (console.log('FOCUSOUT', new Error()), emit('hide'))"


Frizi · 2024-04-18T12:49:05Z

app/gui2/shared/util/data/result.ts

  | { ok: true; value: T }
  | { ok: false; error: ResultError<E> }

-export function Ok<T>(data: T): Result<T, never> {
+export function Ok(): Result<void, never>


This should be undefined, not void. We actually guarantee that return type is undefined. void is not exactly the same as undefined when used in function return position, it should be treated more like "the return type can be anything, and you shouldn't look at it". We do use Promise<void> as a return from many functions already, but there that semantic mostly makes sense (e.g. in LS requests, which we kinda assume return nothing interesting, but no guarantees what it exactly is).

See https://www.typescriptlang.org/docs/handbook/2/functions.html#assignability-of-functions

Today's typescript is a bit more strict about this type than it used to, but there are still some subtle ways where it can bite you.

Frizi · 2024-04-18T12:59:34Z

app/gui2/src/components/GraphEditor/toasts.ts

-  projectStore.executionContext.on('executionComplete', () => toastExecutionFailed.dismiss())
+  projectStore.lsRpcConnection.client.onError((e) =>
+    toastLspError.show(`Language server error: ${e}`),
+  ),


remove comma and reformat

It is the reformatter who puts this comma.

Frizi · 2024-04-18T13:11:05Z

app/gui2/ydoc-server/languageServerSession.ts

+            this.state satisfies never
+            return Ok()


I know it was originally like that, but it would be better to use assertNever here. Now we have it in shared, so it shouldn't be a problem to change.

Frizi · 2024-04-18T13:12:15Z

app/gui2/ydoc-server/languageServerSession.ts

+            })
+          }
+          default: {
+            this.state satisfies never


Another place for assertNever.

farmaazon added 14 commits April 8, 2024 16:01

WIPP

96436d7

Not compiling WIP

114ee09

WIP

f91cde8

Try to fix imports

21b668b

Merge branch 'develop' of github.com:enso-org/enso into wip/farmaazon…

b6a9677

…/reconnecting

Add reconnecting to data socket

3a79724

Fix lint

454b0a3

Fix unit test

13f1f59

WIP

0ad04cf

Fix closing LS

36bbf56

A bit of docs

30f85ea

Make LS disposed error non-fatal

d087486

Merge branch 'develop' of github.com:enso-org/enso into wip/farmaazon…

da9f87c

…/reconnecting

Remove stray logs

34c3278

farmaazon requested review from Frizi, vitvakatu and kazcw as code owners April 12, 2024 15:04

farmaazon added CI: No changelog needed Do not require a changelog entry for this PR. x-new-feature Type: new feature request -gui labels Apr 12, 2024

farmaazon added 7 commits April 12, 2024 17:11

Fix

a400794

Fix lint

671179e

Fix transport mock

d7df880

Fixes

9d8979b

Last touches

a696f13

Merge branch 'develop' of github.com:enso-org/enso into wip/farmaazon…

19f3e73

…/reconnecting

Fix lint

b4607d2

hubertp mentioned this pull request Apr 16, 2024

Reconnect with LS (engine edition) #9710

Closed

Fix e2e test

1a73aa4

Frizi requested changes Apr 17, 2024

View reviewed changes

farmaazon added 3 commits April 17, 2024 15:43

Apply part of @Frizi review

d21a58a

Remove wait from tests. And fix some bugs in by the way

1caa16e

Apply rest of the review

955cdbc

farmaazon requested a review from Frizi April 18, 2024 07:56

farmaazon added 2 commits April 18, 2024 10:50

Send AI prompt as json

aada8ac

Another E2E tests fix

4bfd544

Frizi approved these changes Apr 18, 2024

View reviewed changes

farmaazon added 2 commits April 18, 2024 15:49

Apply next round of review

7e3690c

Fix compilation

5eaa50e

farmaazon added the CI: Ready to merge This PR is eligible for automatic merge label Apr 19, 2024

mergify bot merged commit de406c6 into develop Apr 19, 2024
36 of 37 checks passed

mergify bot deleted the wip/farmaazon/reconnecting branch April 19, 2024 13:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatic reconnect with Language Server. #9691

Automatic reconnect with Language Server. #9691

farmaazon commented Apr 12, 2024 •

edited

Loading

Frizi Apr 16, 2024

Frizi Apr 16, 2024

farmaazon Apr 17, 2024

Frizi Apr 16, 2024

farmaazon Apr 17, 2024 •

edited

Loading

Frizi Apr 17, 2024

farmaazon Apr 17, 2024

Frizi Apr 17, 2024

Frizi Apr 17, 2024

farmaazon Apr 17, 2024 •

edited

Loading

Frizi Apr 17, 2024

farmaazon commented Apr 18, 2024

Frizi left a comment

Frizi Apr 18, 2024

Frizi Apr 18, 2024

Frizi Apr 18, 2024

farmaazon Apr 18, 2024

Frizi Apr 18, 2024

Frizi Apr 18, 2024

		groups.error.log('Cannot read component groups')
		console.error('Continuing without gruops')

Automatic reconnect with Language Server. #9691

Automatic reconnect with Language Server. #9691

Conversation

farmaazon commented Apr 12, 2024 • edited Loading

Pull Request Description

Important Notes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

farmaazon Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

farmaazon Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

farmaazon commented Apr 18, 2024

Frizi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

farmaazon commented Apr 12, 2024 •

edited

Loading

farmaazon Apr 17, 2024 •

edited

Loading

farmaazon Apr 17, 2024 •

edited

Loading