Initialize LSP progress token before using it and remove progress for sync plugins

[syphar]

This now initializes a progress token before using it.

I'm don't have much experience with json-rpc & LSP implementation details, the adapted tests & fixtures stem from my little understanding.

I'm happy to add any needed adaptions.

Fixes #325

[ccordoba12]

Thanks @syphar for your prompt response to address issue #325! I left some small suggestions for you, otherwise looks good to me.

[ccordoba12]

@rchl, could you take a look at this one? Thanks!

[rchl]

Though lint would be immune to the case I've mentioned because it doesn't use any input state. It just gets delayed after document was opened or modified and then works on whatever state the document is currently in when it has a chance to run.

This hypothetical issue would rather apply to requests like references or definition whose result is related to the input params provided by the client.

[ccordoba12]

What do you think about this solution?

@syphar, I'm really not sure what to do about this. As @rchl said, this could generate unexpected results if the document state changed with respect to the state used by a request being processed on a thread.

Is there another way to handle this that you can think of?

[syphar]

What do you think about this solution?

@syphar, I'm really not sure what to do about this. As @rchl said, this could generate unexpected results if the document state changed with respect to the state used by a request being processed on a thread.

Is there another way to handle this that you can think of?

I believe I don't know enough about JSON-RPC & LSP in general, and pylsp_jsonrpc specifically.

From what I saw, I wasn't able to receive the answer for the initialize-token request, while synchronously handling the request from the editor.

So while I definitely don't rule out the possibility that there is a better way of doing this, I can't see it right now.

I can leave this on my list, but finding a way will first need some reading & testing on how the protocol & the implementation works. If no one can provide a hint ;)

[ccordoba12]

For example, in theory, the references request would come but not get handled immediately and then didChange notification would come that would modify the document state. I can theorize a bit and imagine that this could result in results returned from those requests getting out of sync with the document state.

I've run this scenario using definitions instead of references and adding time.sleep(5) in pylsp_definitions to simulate a long running operation. After requesting for an object's definition, I removed the line that contained that object to check how pylsp handled the situation.

What I found is precisely what @rchl predicted: an error in the server because the line is not available anymore.

So while I definitely don't rule out the possibility that there is a better way of doing this, I can't see it right now.
I can leave this on my list, but finding a way will first need some reading & testing on how the protocol & the implementation works. If no one can provide a hint ;)

@syphar, I think for this to work the server needs to be fully async-aware. There's some work to make python-lsp-jsonrpc async here. We would need to do something similar for the server too.

And given what @rchl reported on issue #325, I think the best we can do for now is to remove progress reporting (with the exception of linting, perhaps) because it's not working as expected.

[syphar]

For example, in theory, the references request would come but not get handled immediately and then didChange notification would come that would modify the document state. I can theorize a bit and imagine that this could result in results returned from those requests getting out of sync with the document state.

I've run this scenario using definitions instead of references and adding time.sleep(5) in pylsp_definitions to simulate a long running operation. After requesting for an object's definition, I removed the line that contained that object to check how pylsp handled the situation.

What I found is precisely what @rchl predicted: an error in the server because the line is not available anymore.

Thank you for testing!

So while I definitely don't rule out the possibility that there is a better way of doing this, I can't see it right now.
I can leave this on my list, but finding a way will first need some reading & testing on how the protocol & the implementation works. If no one can provide a hint ;)
@syphar, I think for this to work the server needs to be fully async-aware. There's some work to make python-lsp-jsonrpc async here. We would need to do something similar for the server too.

Thanks for the info, this sounds like some more work.

And given what @rchl reported on issue #325, I think the best we can do for now is to remove progress reporting (with the exception of linting, perhaps) because it's not working as expected.

That's up to you to decide of course. In my mind, even though we're not following the standard, progress reporting works (without this PR), with the exception of sublime text.

[ccordoba12]

In my mind, even though we're not following the standard, progress reporting works (without this PR)

But the standard is there for a reason. So, to clarify my previous point: I think we should leave progress reporting, along with the changes you've done in this PR to make it LSP compliant. But we should remove it from definitions, references, formatting and renames until the server is async-aware (that would preserve linting, which is the request you need it for, right?)

[syphar]

In my mind, even though we're not following the standard, progress reporting works (without this PR)

But the standard is there for a reason. So, to clarify my previous point: I think we should leave progress reporting, along with the changes you've done in this PR to make it LSP compliant.

I assume you mean the changes from this PR, without the threading / async change. I can probably invest some time into this the next days.

The only concern I would personally have with this is that the implementation of progress reporting would just hang when used in the wrong way (in a sync handler), which could be confusing for plugin developers. I'll check if I can make it fail in these cases.

But we should remove it from definitions, references, formatting and renames until the server is async-aware (that would preserve linting, which is the request you need it for, right?)

Personally I needed it primarily for linting, but also for definitions. But I totally understand your reasoning from a project viewpoint, so that's fine with me.

To elaborate on my need with definitions:
in a mid-sized project I'm working on (400k LOC, + 300 dependencies), "go to definition" sometimes takes 5-10 seconds, or even longer. So after pressing the corresponding keymap, I can't quickly differentiate between the LSP not being able to find a definition, and it just taking a longer time.

But I'll find another way, perhaps adding status reporting as a lua-wrapper, and/or showing an error message when no definition can be found.

[ccordoba12]

I assume you mean the changes from this PR, without the threading / async change.

Right.

I can probably invest some time into this the next days.

Great! Thanks for your help with that.

The only concern I would personally have with this is that the implementation of progress reporting would just hang when used in the wrong way (in a sync handler), which could be confusing for plugin developers. I'll check if I can make it fail in these cases.

Good point. If you don't manage to make it fail, then we can add a mention to our docs (which are under the docs folder) to say that progress reporting doesn't work for sync handlers.

But I'll find another way, perhaps adding status reporting as a lua-wrapper, and/or showing an error message when no definition can be found.

Ok, thanks for understanding.

[syphar]

@ccordoba12 so, I invested some time into this PR today.

• I reverted definitions, format_range and references back to synchronous calls, since they get some document position
• I left format_document async, since similar to lint, it only gets the full document.
• I removed progress reporting for the plugins that are handled synchronously
• ( I'm not sure about this one): I added timeout=1.0 to the create-token request. This would kick in for the case that someone tries to do progress reporting in a synchronous handler. I didn't see any other way to get an error for this situation. This would also error when the editor is just too slow.
• I also changed the logic, so when token-creation is failing, or timing out, we just don't do progress reporting.

What do you think about these changes now?

[ccordoba12]

A couple of small suggestions and a question, otherwise looks good to me @syphar.

[ccordoba12]

What do you think about these changes now?

I think this is almost ready. Thanks @syphar for your continued work on this!

@rchl, could you test this PR against Sublime to see if things work as expected now? Thanks!

[rchl]

If the client provides a workDoneToken in the request then it can be used instead of creating new token.

I guess it's technically not an error to still create a token like this code does but just saying.

[rchl]

BTW. We've implemented a workaround in ST some time ago for servers that don't create a token so it was not a problem anymore. But those changes appear to work fine too.

[ccordoba12]

@rchl, are you ok with this now? Just checking before merging.

[rchl]

yeah, fine by me

[ccordoba12]

Thanks @syphar!