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

Support workspace/configuration request for every server initilize #510

Closed
July541 opened this issue Aug 6, 2023 · 14 comments · Fixed by #512
Closed

Support workspace/configuration request for every server initilize #510

July541 opened this issue Aug 6, 2023 · 14 comments · Fixed by #512

Comments

@July541
Copy link
Contributor

July541 commented Aug 6, 2023

In the latest release of vscode-language-server-node, they don't send workspace/didChangeConfiguration more when the server is initialized, which causes haskell/vscode-haskell#920.

Following the comment of the SynchronizeOptions below, they suggest using workspace/configuration, so I'm thinking to extend ServerDefinition with a new field to send workspace/configuration while the server is initiated.

data ServerDefinition config = forall m a.

https://github.com/microsoft/vscode-languageserver-node/blob/f2ff7d55464a1f58f978cb6635bd8865f050553c/client/src/common/configuration.ts#L132

@michaelpj
Copy link
Collaborator

This is actually pretty bad. We can't send workspace/configuration requests during initialize. The "solution" seems to be "use dynamic registration for everything so you can request the configuration"?? This is very much not the model that we use, and probably requires significant re-architecting.

Possibly we can get away with doing this work when we receive the initialized notification from the client, but I'm really not sure.

I have read several of the issues about this and honestly I'm very confused about how this is supposed to work :(

@michaelpj
Copy link
Collaborator

Okay, so I think we could try and do something like what is described here as what rust-analyzer does: microsoft/language-server-protocol#972 (comment)

Namely:

  • The server definition should specify the "section" that its configuration is in
  • We must have a default value for the config
  • If initializationOptions contains a member with the name of the section, use that as the initial config
  • In the handler for initialized, we send workspace/configuration for the whole section, and replace the config with that (maybe don't do this if it was in initializationOptions? IDK)
  • Whenever we receive workspace/didChangeConfiguration, ignore the value and refresh the whole config with workspace/configuration.

@July541
Copy link
Contributor Author

July541 commented Aug 6, 2023

Okay, so I think we could try and do something like what is described here as what rust-analyzer does

That is what I want.

I think the only problem is I'm not sure if we can send workspace/configuration after initialized. I'll try this several days later if you don't have time:)

Night is long, let me try it now:)

@July541 July541 closed this as completed Aug 6, 2023
@michaelpj michaelpj reopened this Aug 7, 2023
@michaelpj
Copy link
Collaborator

You're explicitly allowed to send workspace/configuration in the handler for initialized: microsoft/language-server-protocol#567 (comment)

The only thing is that I guess the client might start sending you requests during that time, so you probably have to be responsive. But we do already have the facility to change the config dynamically, so it shouldn't be too bad.

@michaelpj
Copy link
Collaborator

It just means that e.g. if you somehow hit a formatting request immediately after initialization you might get the default behaviour. But that probably just means that the default behaviour should be "no formatting", I guess.

@michaelpj
Copy link
Collaborator

Sounds like indeed the answer is "tough, accept it": microsoft/language-server-protocol#1006 (comment)

@July541
Copy link
Contributor Author

July541 commented Aug 7, 2023

BTW, I'm trying to find a way to ensure the client will send workspace/didChangConfiguration after restarting.

@michaelpj
Copy link
Collaborator

I think we can't count on that, though, so we probably need to handle the other case anyway.

@July541
Copy link
Contributor Author

July541 commented Aug 7, 2023

After reading some lsp code I found that patching handlers for SMethod_Initialized seems very hard in lsp side.

Is it possible to add a field about workspace/configuration handler, and then run it after SMethod_Initialized handler?

@michaelpj
Copy link
Collaborator

So, we already have this callback that should give us all the information we need to process config:

, onConfigurationChange :: config -> J.Value -> Either T.Text config

I think the only thing we need to be told additionally is the "section".

I agree it's a little tricky in lsp: the idea of having multiple handlers for things and then gluing them together is a HLS thing. So maybe we just want to provide people with helper functions to call in doInitialize and an Initialized handler. But I'm not really sure. It would be very nice if it just magically worked...

@July541
Copy link
Contributor Author

July541 commented Aug 7, 2023

So, we already have this callback that should give us all the information we need to process config:

Seems not this, onConfigurationChange is for handling the didChangeConfiguration notification. I think we need a function to send workspace/configuration request.

Left this up and see if haskell/haskell-language-server#3745 works firstly...

@michaelpj
Copy link
Collaborator

Seems not this, onConfigurationChange is for handling the didChangeConfiguration notification

I mean, that tells us what to do when we get a new config. Actually implementing it we can probably pull out a part of handleConfigChange.

fendor added a commit to fendor/vscode-haskell that referenced this issue Aug 9, 2023
It causes troubles due to following the newer LSP spec which HLS is not
ready for. For details, see haskell#920
and haskell/lsp#510, etc...
@Sarrus1
Copy link

Sarrus1 commented Feb 8, 2024

Sounds like indeed the answer is "tough, accept it": microsoft/language-server-protocol#1006 (comment)

Hello, I have also struggled with this issue on my LSP implementation. My workaround was to:

  • Send a workspace/configuration request right after the initialized notification is received.
  • Wait for the workspace/configuration and store any other notification/request in a buffer.
  • Receive workspace/configuration, then update the config.
  • Process the buffered notifications/requests.

This is hacky, but it seems better than updating the config after we received some requests.
I reasoned that if the server has not received the config yet, it is not properly initialized.

I see that this is closed, but maybe this can help someone with the same issue.

@michaelpj
Copy link
Collaborator

I think we now handle this pretty well, we send workspace/configuration during the handling of initialized. We also generally handle notification sequentially and don't process anything else concurrently with them, since they often update state, so we basically don't do anything until we're done getting the initial configuration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants