Speed up initialization by not waiting for nimsuggest to start

[moigagoo]

No description provided.

[moigagoo]

@jmgomez I know this is controversial but it does fix the issue for nim-libp2p while not breaking the tests (LSP tests were not modified, MCP tests had to be updated to handle the nimsuggest notification).

[jmgomez]

It only means that the test doesnt cover what will break.

[moigagoo]

It only means that the test doesnt cover what will break.

Makes sense :-)

What is the edge case in question though?

[moigagoo]

What is the edge case in question though?

I mean to cover it with a test and maybe fox it while retaining the init speedup.

[moigagoo]

I think I now understand why await is used there. With asyncSpawn, there's nothing preventing another nimsuggest from being spawn when didOpen request arrives. In fact, this is exactly what happens: two nimsuggests are spawned at startup, one of them being eliminated later as idle.

This is of course not so great 😕 I think I can hack around that with a semaphor or something like that but the solution is beginning to look less sexy with more hacks.

[jmgomez]

Yeah, as mentioned in the issue the race cond come from the client thinking the server is init when it isnt. Anyways, whats the point of having the langserver ready without nimsuggest being ready? It doesnt provide func without it

[moigagoo]

whats the point of having the langserver ready without nimsuggest being ready? It doesnt provide func without it

The point is that LSP initialization is basically a handshake where the server simply tells the client about its capabilities. AFAIU this is expected to be close to instant, otherwise LSP client just considers the initialization failed. I'm seeing this with Helix, VS Code, NeoVim—basically any LSP client so far. The same happens with MCP clients (tested with OpenCode as I use it the most nowadays).

It is more correct for the didOpen and other request handlers to await on the nimsuggest start that the initilaize handler because you don't need nimsuggest to respond to an initialization request but you need it to handle incoming requests.

[jmgomez]

IIRC didOpen is just a notification and the client doenst wait ( I may be wrong it has been a while). Anyways, you need to inform the capabilities, both the dump and the get capabilities from the ns should be instant, if they are not, there is an issue with them which needs to be fixed

[moigagoo]

IIRC didOpen is just a notification and the client doenst wait ( I may be wrong it has been a while).

The server spawns nimsuggest on didOpen if it's not available for this projectFile yet. The client doesn't wait for it, that's true, but I don't want another nimsuggest to start while the previous one is still being started.

Anyways, you need to inform the capabilities,

Yes, and they are currently (on master) ready to be sent to the client before nimsuggest is spawned: https://github.com/nim-lang/langserver/blob/master/routes/lsp.nim#L62-L108

This is why we're actually ready to respond to the client without waiting for nimsuggest to start.

both the dump and the get capabilities from the ns should be instant, if they are not, there is an issue with them which needs to be fixed

Sure, slow nimble dump is an issue per se and it should be fixed.

But conceptually spawning nimsuggest can be a long operation, at least longer that the client is willing to wait. It's practically a race now: if nimsuggest manages to start quickly enough, the connection initializes fine, otherwise (due to large codebase, nimble bug, slow network, slow machine, whatever) the initialization fails.

If want to guarantee that initialization always works. The first response from the server may be slow (again, due to large codebase, nimble bug, etc.) but at least you will have it working.

Using asyncSpawn is not the correct solution though as we basically just leaving nimsuggest spawning unattended. I'll send a better fix soon (testing now).

[moigagoo]

Instead of asyncSpawning nimsuggest, I'm now creating an awaitable nimsuggestInit future in LanguageServer. In didOpen and callTool I await on it, which guarantees a) that we don't spawn extra nimsuggest instances and b) no calls to nimsuggest are made before it's ready.