Server capability for configuration change notifications?

[michaelpj]

At the moment the recommended way for servers to opt-in to configuration change notifications is to dynamically register for workspace/didChangeConfiguration (see the text here: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_configuration). This is inconsistent with how things work normally: usually the server indicates that it wants the client to behave a particular way by setting a server capability.

I also think this deserves to be a capability: the model where the server caches the results of workspace/configuration is very reasonable (sending a request every single time you look at configuration is quite expensive!), and that requires some kind of notification from the client when the configuration changes and so the cache is invalid.

Concretely I propose:

interface ServerCapabilities {
  ...
  workspace? : {
     ...
    configuration? : ConfigurationOptions;
  };
}

interface ConfigurationOptions {
  /** A list of configuration section that the server is interested in. 
     If omitted then the client should assume that the server cares about the full configuration.
  */
  section? : [string]
  /** Whether or not the server needs to be notified when the configuration changes. 
    If omitted then it is assumed to be 'false' 
  */
  notify? : bool
  /** Whether or not configuration change notifications must include the changed settings or not. 
    If omitted then it is assumed to be false.
  */
  includeSettings? : bool
}

[michaelpj]

Indeed, I would argue that changing the behaviour without providing a server capability is a breaking change. VS Code silently changing how it works means that old language servers are simply broken, which violates the normally strong approach to backwards compatibility that the LSP spec has.

[michaelpj]

Maybe there should also be a client capability indicating the client's preferred configuration interaction model.

[michaelpj]

I would be fine if VS Code were to give an error if the server doesn't conform to its desired model, but silently failing to work is really quite bad.

[dbaeumer]

There is a difference between the old push model and the new pull model. The old push model was always client based (there was never a server capability for it) and it pushed settings values as well. This was actually bad since a server could never decided whether it wanted to receive these updates or not.

The old push model (without server nor dynamic registration) got replace by the new pull model. Major reason for this is that a pull model can support scopes where a push model can't. Do to the fact that the new pull model has scopes the change event can't carry any data in a reasonable way since that data depends on the scope which the client must not necessarily know. This is why the dynamic registration doesn't support any options since the dynamic registration got introduced together with the pull model.

I will add a sentence to the spec to make this more clear.

[michaelpj]

The old push model (without server nor dynamic registration) got replace by the new pull model.

But there is no configurability for this, so old language servers are just broken with new clients and new servers may not work with old clients. Normally you're very good about this so it's a shame we don't have anything here!

This is why the dynamic registration doesn't support any options since the dynamic registration got introduced together with the pull model.

Is the metamodel wrong? The spec doesn't give the type for the registration params either.

[dbaeumer]

But there is no configurability for this, so old language servers are just broken with new clients and new servers may not work with old clients. Normally you're very good about this so it's a shame we don't have anything here!

Old clients can still support the push model. There is no reason why clients shouldn't. So why do you think that old servers are broken?

Is the metamodel wrong? The spec doesn't give the type for the registration params either.

I will dig into this and I guess here is what is happening. In the VS Code implementation the client options will create a DidChangeConfigurationRegistrationOptions under the hood and that implementation detail leak into the meta model. As you can see none of the specs have that type. And the documentation in the meta model even say The notification contains\nthe changed configuration as defined by the language client. as it was with the old push model.

[dbaeumer]

I know that the model is not easy but it got defined that way to be backwards compatible with older versions before we introduced the pull model.

[michaelpj]

Old clients can still support the push model. There is no reason why clients shouldn't. So why do you think that old servers are broken?

(did you mean "new clients can still support the push model"?)

An old server used with a new client will not work because the new client will use the pull model and so won't send change notifications. At the very least, new clients might require dynamically registering for didChangeConfiguration, which we didn't have to do in the past.

In fact, I started down this particular rabbit hole because someone reported that vscode had stopped sending didChangeConfiguration notifications to servers written using our lsp library. I don't know yet exactly why, but I suspect it might be that we don't yet do dynamic registration for didChangeConfiguration.

I'm not saying you shouldn't move on, I'm just pointing out that usually you are very careful about making new behaviour opt-in, and this is an exception and that is probably going to lead to breakage. If we want things not to break then we need to insist that clients support the push model if the sever needs it, and I think the most natural way to signal that is with a server capability :shrug:

[dbaeumer]

(did you mean "new clients can still support the push model"?)

Yes, since the push model is still in the spec. To not break we even don't deprecate things. The VS Code client still supports the old model to push changes.

As I tried to say earlier the push model was never an opt in from the server in terms of which settings to push. That was always a client decision. So there could even in the past easily be a client A not pushing actual settings changes and a client B doing so. There was also no guarantee that if both clients push they push the same settings in the same structure. So this was always very special code on the client.

All this resulted to a very flawed push settings model which we tried to fix with the pull model. All I can see as a problem right now is that if a client is configured to push and the server pulls the client pushes settings changes which the server has to ignore as the spec says that in a pull model the change event should be empty.

I do agree that the meta model is currently incorrect since it doesn't match the spec but I would rather fix the meta model than improving a feature that is broken in the first place. I will also look if in the implementation I can ensure that if the server pull the settings change event is always empty as the spec says.

[michaelpj]

I see. I guess it was possible before for clients to not push settings. But in practice they did. And so changing to not do that breaks servers.

I guess the dynamic registration does give me what I want which is for servers to be able to state that they need the push model.

Note that what we're actually doing, and what I think will be very common, is a hybrid: get notifications when things have changed, and then pull and store the updated config. That relies on both parts.

[dbaeumer]

Note that what we're actually doing, and what I think will be very common, is a hybrid: get notifications when things have changed, and then pull and store the updated config. That relies on both parts.

This is actually how it is supposed to work in the new model and how I use it in ESLint for example. If you don't store the config then you wouldn't need the event since the get configuration call to the client would always return the current values.

[michaelpj]

Right, but that's going to have crazy high overhead for most applications IMO.

Anyway, that wasn't clear to me - the current prose makes it look like didChangeConfiguration is almost deprecated, and that registering for it is a workaround for not using the pull model "properly". Glad to hear it's the intended route!