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

client: option to suppress language client start error message popup #1011

Closed
hyangah opened this issue Jun 23, 2022 · 5 comments
Closed

client: option to suppress language client start error message popup #1011

hyangah opened this issue Jun 23, 2022 · 5 comments
Labels
feature-request Request for new features or functionality help wanted Issues identified as good community contribution opportunities
Milestone

Comments

@hyangah
Copy link
Contributor

hyangah commented Jun 23, 2022

Recently we upgraded our language client to support LSP 3.17, and noticed certain error conditions in language client start up result in multiple popups even though we set RevealOutputChannelOn.Never.

Screen Shot 2022-06-23 at 7 41 24 PM

My guess is this is a change from #824

Our extension has custom initializationFailedHandler and errorHandler that retry or show a customized error notification already, so it would be nice if we can suppress them.

@dbaeumer
Copy link
Member

Actually, it is on purpose that these errors are not suppressed since they indicate a deeper underlying problem (e.g. the server crashed or didn't start at all).

In that latest you can customize the message that is shown in these cases. See

export type ErrorHandlerResult = {

Can you explain why this is a normal behavior for your language server?

@dbaeumer dbaeumer added the info-needed Issue requires more information from poster label Jun 24, 2022
@hyangah
Copy link
Contributor Author

hyangah commented Jun 24, 2022

Can you explain why this is a normal behavior for your language server?

Some of our advanced users launch our language server in what we call proxy mode.
The proxy mode language server launched by vscode connects to a remote language server process (specified by either tcp or unix domain socket address given with a flag, or through the server's own discovery mechanism). This mode is useful when we need to debug/test our language server under development, or need to share one language server instance with other editors. In this mode, if the language server cannot connect to the remote server (e.g. the remote server is not up yet, or it crashed - this can occur often during development) or even the proxy crashes, we want the client to retry a few times. In case the remote server and the client race in start up, this retry reduces noise.

In that latest you can customize the message that is shown in these cases

Can we customize the followup action? We want more than the "Go to output" option - bug report generation.

When the user chooses the option, the extension collects necessary data (anonymized server crash trace, system info) and pre-fills a github issue.

Finally, I tested this proxy mode setup after removing all our custom initializationFailedHandler and errorHandler (so default behavior, but still with revealOutputChannelOn: Never), and observed multiple error popups are shown to user. Some error popups seemed deduped(?) but it resulted in an interesting flash effect while the client is retrying behind the scene. I wish we could suppress some of the popups.

Screen Shot 2022-06-24 at 9 11 42 AM

@dbaeumer
Copy link
Member

I see your point. What we could do is the following: add an optional handled property to the Error and Cloe handler. If present and set to true we could not show any error dialog.

See code:

export type ErrorHandlerResult = {
and
export type CloseHandlerResult = {

Would you like to give this a try and see whether it would improve the situation for you. We would also need to do something for the InitializationFailedHandler but that would be breaking right now and might need to wait until we have a new major version.

gopherbot pushed a commit to golang/vscode-go that referenced this issue Jun 28, 2022
…tom error handler

From vscode-languageclient v8.0.x (LSP 3.17), the language client reports the start
errors specially and surfaces them as user visible popups regardless revealOutputChannelOn
setting. Thus, our error popups are no longer necessary. Remove this.

When an error occurs during initialization, the connection to the server shuts
down which triggers the close handler. Previously, we suggested gopls issue
report from both initializationFailedHandler and errorHandler.closed handler.
That now results in two gopls issue report suggestions. Instead, stash the
initialization failure error, and handle the suggestion prompt from one place.

Note - in case of initializtion error, there is no point of retrying 5 times.

We also explicitly set the error & connection close handlers messages to empty
when requesting for continue/restart action, which will suppresses the default
error message popups.

While we are here, we improve the go status bar's language server state report
by updating it when the language server connection close is obeserved.

Updates microsoft/vscode-languageserver-node#1011
Fixes #2300

Change-Id: I8b20cf11ebb61ab474950440fc46ff23f7b0b826
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/414457
Reviewed-by: Jamal Carvalho <jamal@golang.org>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
gopherbot pushed a commit to golang/vscode-go that referenced this issue Jun 29, 2022
…s from custom error handler

From vscode-languageclient v8.0.x (LSP 3.17), the language client reports the start
errors specially and surfaces them as user visible popups regardless revealOutputChannelOn
setting. Thus, our error popups are no longer necessary. Remove this.

When an error occurs during initialization, the connection to the server shuts
down which triggers the close handler. Previously, we suggested gopls issue
report from both initializationFailedHandler and errorHandler.closed handler.
That now results in two gopls issue report suggestions. Instead, stash the
initialization failure error, and handle the suggestion prompt from one place.

Note - in case of initializtion error, there is no point of retrying 5 times.

We also explicitly set the error & connection close handlers messages to empty
when requesting for continue/restart action, which will suppresses the default
error message popups.

While we are here, we improve the go status bar's language server state report
by updating it when the language server connection close is obeserved.

Updates microsoft/vscode-languageserver-node#1011
Fixes #2300

Change-Id: I8b20cf11ebb61ab474950440fc46ff23f7b0b826
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/414457
Reviewed-by: Jamal Carvalho <jamal@golang.org>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
(cherry picked from commit 99369b6)
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/414460
Reviewed-by: Suzy Mueller <suzmue@golang.org>
@hyangah
Copy link
Contributor Author

hyangah commented Jul 13, 2022

Thanks @dbaeumer handled property can help.

Meanwhile I found explicitly specifying an empty message (return { message: '', action: CloseAction.Restart}) suppresses the popup window. I am not sure if that's intentional behavior nor what is suppressing this popup. (lsp client or vscode??) Can we make this a "feature" by documenting it?

However, I still couldn't avoid some of the forced popups in certain cases.

See this code that handles closed connection (e.g. connection closed by server):

let handlerResult: CloseHandlerResult = { action: CloseAction.DoNotRestart };
if (this.$state !== ClientState.Stopping) {
try {
handlerResult = this._clientOptions.errorHandler!.closed();
} catch (error) {
// Ignore errors coming from the error handler.
}
}
this._connection = undefined;
if (handlerResult.action === CloseAction.DoNotRestart) {
this.error(handlerResult.message ?? 'Connection to server got closed. Server will not be restarted.', undefined, 'force');

If the client is also in the Stopping state when the server-side connection close is detected first,
clientOptions.errorHandler.closed will not be called. As a result, handlerResult.action remains as the default DoNotRestart and the message in line 1470 will be reported with 'force' option.

I think this problem will be present even with your handled property proposal. How about skipping the forced popup if the client-side shutdown is initiated too?

@dbaeumer dbaeumer added feature request help wanted Issues identified as good community contribution opportunities and removed info-needed Issue requires more information from poster labels Dec 6, 2022
@dbaeumer dbaeumer modified the milestone: On Deck Dec 6, 2022
jpogran added a commit to hashicorp/vscode-terraform that referenced this issue Jan 24, 2023
The vscode-languageclient library introduced breaking API changes in the 8.X version that require changes to our extension to use:

- Remove use of onReady() to wait for start of the LS and use start() instead
- All features that implement StaticFeature need to use the updated interface

In addition to the above were interface changes to ErrorHandler. In applying these changes, it introduced duplicate error messages when loading the extension with incorrect settings. In effect, turning 1 or 2 messages into 3 or 4, depending on what was misconfigured. These cannot be customized: microsoft/vscode-languageserver-node#1011

This commit solves this by removing use of the ExtensionErrorHandler class and instead using inline methods for both initializationFailedHandler and errorHandler to detect failed extension errors. It avoids the duplicate methods by not using the ErrorAction message property and instead keeping track of whether or not an InitializeError was detected.
jpogran added a commit to hashicorp/vscode-terraform that referenced this issue Jan 25, 2023
The vscode-languageclient library introduced breaking API changes in the 8.X version that require changes to our extension to use:

- Remove use of onReady() to wait for start of the LS and use start() instead
- All features that implement StaticFeature need to use the updated interface

In addition to the above were interface changes to ErrorHandler. In applying these changes, it introduced duplicate error messages when loading the extension with incorrect settings. In effect, turning 1 or 2 messages into 3 or 4, depending on what was misconfigured. These cannot be customized: microsoft/vscode-languageserver-node#1011

This commit solves this by removing use of the ExtensionErrorHandler class and instead using inline methods for both initializationFailedHandler and errorHandler to detect failed extension errors. It avoids the duplicate methods by not using the ErrorAction message property and instead keeping track of whether or not an InitializeError was detected.
jpogran added a commit to hashicorp/vscode-terraform that referenced this issue Feb 14, 2023
The vscode-languageclient library introduced breaking API changes in the 8.X version that require changes to our extension to use:

- Remove use of onReady() to wait for start of the LS and use start() instead
- All features that implement StaticFeature need to use the updated interface

In addition to the above were interface changes to ErrorHandler. In applying these changes, it introduced duplicate error messages when loading the extension with incorrect settings. In effect, turning 1 or 2 messages into 3 or 4, depending on what was misconfigured. These cannot be customized: microsoft/vscode-languageserver-node#1011

This commit solves this by removing use of the ExtensionErrorHandler class and instead using inline methods for both initializationFailedHandler and errorHandler to detect failed extension errors. It avoids the duplicate methods by not using the ErrorAction message property and instead keeping track of whether or not an InitializeError was detected.
@dbaeumer dbaeumer added feature-request Request for new features or functionality and removed feature request labels Nov 17, 2023
@dbaeumer
Copy link
Member

An handled property got added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for new features or functionality help wanted Issues identified as good community contribution opportunities
Projects
None yet
Development

No branches or pull requests

2 participants