Skip to content
This repository was archived by the owner on Jul 9, 2025. It is now read-only.

Conversation

@zhixzhan
Copy link
Contributor

@zhixzhan zhixzhan commented Jun 2, 2020

Description

LG/LU editor use WebSocket connect to LSP server, this may failed when page is idle or network is poor, and at this moment inline editor's inside suggestion/check would not work.

we have auto re-connection, so mute the failed alert sounds reasonable.

Task Item

close #3167

Screenshots

Connection lost & resume after editor open.

Untitled2

Connection lost & resume before editor open.

Untitled3

@coveralls
Copy link

coveralls commented Jun 2, 2020

Coverage Status

Coverage remained the same at 38.907% when pulling b79b1f5 on zhixzhan/lsp-connection-error into 2eba21d on master.

Copy link
Contributor

@a-b-r-o-w-n a-b-r-o-w-n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd like to see something that clearly shows that these errors are, in fact, connection errors.

In the description you mention we already have retry logic, so it's a little confusing to me that this code would throw. I'm not super familiar with the language servers / monaco though so I take my thoughts lightly. At minimum I would expect something like:

try {
  await languageClient.onReady();
  languageClient.sendRequest('initializeDocuments', { uri, luOption });
} catch (err) {
  if (err instanceof ConnectionError) {
    await languageClient.reconnect();
  }
}

This code could already be happening somewhere, but the error manifestation seems odd to me if so.

@a-b-r-o-w-n a-b-r-o-w-n changed the title fix: something get wrong alert. fix: suppress websocket error after connection lost Jun 2, 2020
@a-b-r-o-w-n a-b-r-o-w-n self-assigned this Jun 2, 2020
@zhixzhan
Copy link
Contributor Author

zhixzhan commented Jun 3, 2020

I think I'd like to see something that clearly shows that these errors are, in fact, connection errors.

In the description you mention we already have retry logic, so it's a little confusing to me that this code would throw. I'm not super familiar with the language servers / monaco though so I take my thoughts lightly. At minimum I would expect something like:

try {
  await languageClient.onReady();
  languageClient.sendRequest('initializeDocuments', { uri, luOption });
} catch (err) {
  if (err instanceof ConnectionError) {
    await languageClient.reconnect();
  }
}

This code could already be happening somewhere, but the error manifestation seems odd to me if so.

I did not thought clear. The retry logic only resume connection, didn't re-send request.
the flow looks like

  1. open a LSP editor, initializeDocument sendRequest success.
  2. connection lost
  3. retry connecting
  4. connection resume

If phase-1 failed, retry connecting would not exec initializeDocuments again.

I should let retry cover initialize document, please HOLD.

@zhixzhan
Copy link
Contributor Author

zhixzhan commented Jun 3, 2020

@a-b-r-o-w-n updated, the screen gif shows two connection lost scenarios.

});
}

SendRequestWithRetry(languageClient, 'initializeDocuments', { luOption, uri });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be awaited to ensure the document has been initialized?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zhixzhan I am going to go ahead and take this but please respond here when you get back online and create a follow-up PR if this needs to be fixed.

Copy link
Contributor Author

@zhixzhan zhixzhan Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked code below SendRequestWithRetry(languageClient, 'initializeDocuments', {, nothing relied on it.

One effect is before initializeDocuments success, in LSP editor interface all information needs come from LSP server ( diagnostics/suggustions...) would be empty, looks like writing in a plain text-editor.

@a-b-r-o-w-n a-b-r-o-w-n merged commit 0467744 into master Jun 3, 2020
@a-b-r-o-w-n a-b-r-o-w-n deleted the zhixzhan/lsp-connection-error branch June 3, 2020 16:54
lei9444 pushed a commit to lei9444/BotFramework-Composer-1 that referenced this pull request Jun 15, 2021
* report bug template add `Electron`

* mute LSP WebSocket connection lost pop up error

* refine re-connection

* use SendRequestWithRetry

Co-authored-by: Chris Whitten <[email protected]>
Co-authored-by: Andy Brown <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Something went wrong dialog displays in various contexts

5 participants