Allow to register multiple handlers for shutdown

[felixfbecker]

After spending quite some time today tracking down a bug I noticed the cause was that vscode-languageserver does not allow to register more than one handler for shutdown, and subsequent calls will just override the previous handler silently:

https://sourcegraph.com/github.com/Microsoft/vscode-languageserver-node@fc1a79d05f9c52cbb02d6d172d3e17e1516e59b8/-/blob/server/src/main.ts#L1198:1-1199:1

This seems to also be true for exit and initialize.

I really wonder why the decision was made to provide an event-emitter like interface where you can register handlers at runtime (as opposed to a class/object that provides a single implementation for each method) if you can then only register one method handler for certain (?) methods.

Example use case: You want one connection to proxy or load-balance messages to different other servers. The proxy logic registers a handler for every message to forward them, but the cluster logic also registers a handler to shutdown to for example kill workers. Both handlers override each other.

[felixfbecker]

Okay, looking into this more it appears that this is actually true for every method.

[dbaeumer]

This is actually by design. Since most communication is requests which return a result providing n listeners will raise the question on how to merge the result. Since multiple 'listeners' are unlikely for requests I didn't provide them for notifications either for consistency reasons.

I am happy to make the Emitter public so that people can resuse some code that is available for text document changes.

[felixfbecker]

Yeah, eventually I figured that was the reason for requests. But I think for notifications this would be useful, since they are more like "events". For example, independent parts of the application could register listeners to shutdown for clean up work, or update some internal storage if a file changed etc.

I don't understand what you mean with making the Emitter public?

[dbaeumer]

The sever has an event emitter implementation which is not public right now. So I could make it public so that users that need to have a listener model could simply create an Emitter and register on shutdown handler. For consistency reasons I am reluctant to change this.

[felixfbecker]

So this is only about exporting the class? Is it possible to register a generic handler for all notifications then to emit these on the Emitter?

[dbaeumer]

Yes, it is only about exporting the class.

No, there is currently no support to register a generic handler.

[mhintzke]

Is this issue still not being addressed? It is over 2 years since this was added to the Backlog and I am not seeing a PR referencing the problem.

I opened up my own issue here that asks for the more generalized support for multiple Notification handlers. I see above you said the following:

Since multiple 'listeners' are unlikely for requests I didn't provide them for notifications either for consistency reasons.

As I agree that Requests should probably not have multiple handlers as downstream actions could not be idempotent and result in side effects, I think a model around multiple Notification handlers is perfectly acceptable. In the reactive programming model, a Request is similar to a Command and a Notification is similar to an Event and you can have many Event handlers around your system. This model should play out nicely here as well as different levels of the application might need to respond to Notifications differently.

In my example, my application code has some business requirements around allowing saving a file if there are any script diagnostic issues returned from PSES. I have no interface to fetch those diagnostics and I have no way of handling the "textDocument/publishDiagnostics" Notification because if I do attach a handler it completely removes all of the messaging inside my Monaco Editor itself.

There have since been additions to the library to support "middleware" on this kind of Notification, but this was done at a higher level of abstaction and doesn't support as powerful as a model as just simply having multiple handlers would provide.

[dbaeumer]

@mhintzke13 to not make this inconsistent the current approach is to have only one handler on the connection. If on a higher level (sever or client) need to have n handlers this should come from them. So in your case the right solution is that the client offers an event when diagnostics occur. This is similar to other hooks the client offers and to what is implemented on the server side.

I am open to accept a PR for this on the client side.

[DanTup]

I just hit this issue too. I was very surprised to learn that subscribing to custom notifications multiple times would overwrite each handler. My non-LSP server implementation has lots of code that subscribe to notifications from the server multiple times (for example there are multiple different providers, like Outline, Code Lens, Closing Labels that all want to subscribe to updates for Outline data).

It's really confusing to have an event API that looks very similar to other NodeJS and VS Code APIs but work very differently:

// NodeJS:
// supports multiple handlers
this.socket.on("open", this.foo);

// VS Code:
// supports multiple handlers
vs.workspace.onDidCloseTextDocument(this.foo),

// language-client:
// overwrites existing handler 😔
this.client.onNotification(AnalyzerStatusNotification.type, this.foo);

[vrama628]

Is there any workaround for this? It's inconvenient that listening for e.g. textDocument/publishDiagnostics notifications causes the notifications to not reach other handlers that have previously been registered for that notification type.

It's strange that the JavaScript event emitter pattern was used to implement request handlers in the first place, and even stranger that the reason for why request handlers shouldn't be done with the event emitter pattern was then used to break the event emitter pattern for things that genuinely are events (notifications) where it makes sense to have multiple listeners.

What is the recommendation for how to work around this bug?

[dbaeumer]

The workaround is to do the dispatching yourself.

[vrama628]

The workaround is to do the dispatching yourself.

how does one get access to the previously-existing notification handler in order to dispatch to it in the new handler you register? Is there a method on BaseLanguageClient that returns the previously-registered notification handler?

[dbaeumer]

You are usually the one who registers these handlers. Or are you saying you share the client / server with another extension

[vrama628]

Yes, I'm sharing the client with library code that's outside of my control. My use case is that the editor I'm using (monaco-languageclient) seems to register the onNotification("textDocument/publishDiagnostics") handler in order to listen for and display the text decorations (red squiggly lines, etc) in the editor, and after it registers its own handlers it returns the BaseLanguageClient to me so that I can use it for whatever other functionality I need, such as firing custom LSP commands. If I want to keep track of the diagnostics the language server publishes, the natural thing to do is to add an event listener via onNotification, but that boots out the listener the editor registered and makes it so that text decorations no longer get displayed in the editor.

[dbaeumer]

That use case is currently not supported to keep things consistent with requests.

For diagnostics VS Code's API offers a onDidChangeDiagnostics: Event<DiagnosticChangeEvent> on the language namespace which fires when diagnostic changes in the system.

IMO client should use that event instead of relying on a notification from the server. If the client for example pulls for diagnostics the server would never send a publishDiagnostics notification.