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 new CancellationStrategy capability added to vscode-jsonrpc in StreamjsonRpc #436

Closed
heejaechang opened this issue Mar 27, 2020 · 17 comments · Fixed by #541
Closed
Assignees
Milestone

Comments

@heejaechang
Copy link

heejaechang commented Mar 27, 2020

recently new ability is added to vscode-jsonrpc to support custom cancellation mechanism to jsonrpc
(microsoft/vscode-languageserver-node@b1f6a44)

it was added since the current built-in cancellation mechanism assumes consumers can either run concurrently or asynchronously. any synchronous code can't use the existing cancellation mechanism to cancel ongoing request since, at the time, it checks cancellation request, work is already all done.

some of lsp implementation on the node has synchronous lsp implementation (such as typescript, python) so they need different cancellation mechanisms. for example, typescript uses this trick where it creates a cancellation file/pipe and uses the existence of the file/pipe to indicate whether the current request needed to be canceled. python needs yet another way to implement it since they don't want to customize vscode client for cancellation support as typescript did.

to support these various needs and constraints of each LSP implementation, a way to provide its own cancellation mechanism is added to vscode-jsonrpc.

now, we would like to share the same lsp node server with VS but without this custom cancellation mechanism support, we lose a way to cancel any request in VS.

wonder whether it is possible to support this new ability in StreamJsonRpc as well.

@heejaechang
Copy link
Author

this is code actually matter - microsoft/vscode-languageserver-node@b1f6a44#diff-6a132c34fec94e7b4e8e105aa67fdd15

all other code is just plumbing.

@AArnott
Copy link
Member

AArnott commented Mar 28, 2020

My first impression is I'm surprised a JSON-RPC library would ever implement such a thing, since this simply further enables single-threaded platforms like javascript to avoid being async as they should be.
I'm further amazed that this is pushed into the protocol such that other libraries that would never suffer from such a problem (like StreamJsonRpc) would have to implement it to further support the limited platform and poorly implemented RPC server.

I don't think we'd build in such file/pipe cancellation support directly in StreamJsonRpc since it's not remotable. Providing an extensibility point where you could do it is conceivable, maybe.

I've heard of this file handle cancellation mechanism over a year ago in the javascript world. What has changed recently to make this important in the .NET world now?

@heejaechang
Copy link
Author

heejaechang commented Mar 30, 2020

If both sides are .net (or platform that supports threads), it is fine, but when only one side is .net, it's a problem, especially when VS uses StreamJsonRpc underneath. And whether javascript should embrace async or not is, I guess, up to each implementor as people should use javascript or typescript is up to each people. (for example, using async makes debugging several times harder so there are a lot of people who just don't want to have async in their own code and only uses it at the callback such as node)

And I am not asking adding file-based cancellation in StreamJsonRpc, that is not something added to vscode-jsonrpc as well. It started as that, but in the end, the extensibility point called CancellationStrategy got added so that people can override its behavior. And that's what I am asking.

@AArnott
Copy link
Member

AArnott commented Mar 31, 2020

And whether javascript should embrace async or not is, I guess, up to each implementor as people should use javascript or typescript is up to each people

Async is part of the ECMA spec now. So it's available whether folks use javascript or typescript.

the extensibility point called CancellationStrategy got added so that people can override its behavior. And that's what I am asking.

Alright. I could imagine StreamJsonRpc offering that extensibility point.

But why now? This file handle pattern has been around for a long time.

@heejaechang
Copy link
Author

heejaechang commented Mar 31, 2020

Async is part of the ECMA spec now. So it's available whether folks use javascript or typescript.

I meant it is up to people whether they stick with javascript or move to typescript even if typescript now exists. what I was trying to say was even if async is now part of ECMA, it is up to people whether they embrace it or not.

also, I believe it is completely fine people saying they won't use async over synchronous code as some people say they will stick with javascript even if typescript now exists.

anyway, I was just using javascript vs typescript as figure of speech, so not important.

..

also, I used file-based cancellation as an example of one of custom cancellation logic as well, so please don't hang up on a file-based cancellation too much. what I am asking is a way to customize cancellation logic. not file-based cancellation specifically.

right now, none of LSP protocol forces server to be async or support concurrency. cancellation is the only thing that forces it. so, it feels reasonable to provide a way to customize cancellation logic for people who don't want to or can't do concurrency or async.

...

we are working on moving some python language server to the latest LSP and the whole code is written in a synchronous manner in ts. and we didn't want to go the typescript team's route where they don't use LSP at all and wrote everything themselves. (they also don't use async as far as I know)

instead, we wanted to re-use all existing LSP frameworks (language client, protocol, jsonrpc) and the only thing blocking us was the cancellation.

we already tested it on vscode and it works as expected. now we want to use the same node-based LSP server with VS, and the first thing blocking us is StreamJsonRpc not supporting custom cancellation logic. so that is why I am asking this new feature.

after that, I need to ask VS LanguageClient to expose it so that we can override.

@AArnott
Copy link
Member

AArnott commented Mar 31, 2020

OK, thanks for that added background and timeline.

When the LSP server is on another machine or perhaps a docker container (e.g. VS Code opening a folder within WSL), the client opening a file handle won't have any perceivable impact on the LSP server I think. How do they "remote" this non-remotable cancellation protocol? When an LSP server is on a Live Share host, does the guest send a typical cancellation protocol message to the Host, where the Host then does the proprietary cancellation thing on a per-Server basis (within inside knowledge of what that particular LSP server expects)?

Doesn't the LSP protocol itself describe how cancellation works as using JSON-RPC messages?

@heejaechang
Copy link
Author

heejaechang commented Mar 31, 2020

right now, python doesn't support Cloud Environments and it is a good question on the remote case. but in such a case, without any custom logic, our LSP won't support cancellation anyway at the current form. so nothing gets worse than what we have now.

also, since it is custom logic, we can still send cancellation request + our proprietary cancellation logic so, middle man can just forward it to the other side ignoring our custom logic, if remote side magically knows how to start our node-based lsp server without any input from us, then I guess they should know how to understand our custom cancellation logic (?), if they need our code to start our server in remote, then we can add code there to start our server with knowledge on how to handle remote cancellation?

anyway, good question on remote case, will ask typescript team how they are handling it.

@AArnott
Copy link
Member

AArnott commented Mar 31, 2020

our LSP won't support cancellation anyway at the current form. so nothing gets worse than what we have now.

Are you sure? Because if Live Share is the remoting transport for LSP (which it is) wouldn't it be Live Share that needs to support this cancellation strategy option? Live Share doesn't use StreamJsonRpc for most of its RPC as they have their own implementation. So at very least you may need to look into persuading Live Share to support this as well.

This leaves the all-local case though, where I think Live Share doesn't get involved so StreamJsonRpc would need to support it.

@heejaechang
Copy link
Author

heejaechang commented Mar 31, 2020

for VS live share case, each live share supporting language has an extensibility point where they can provide its own code to connect/create host side (client VS to host VS to lsp server). so I think it is okay.

for vscode, I am not sure how live share works, so I need to investigate a bit. but we don't support live share yet for both vscode or VS. but will support it eventually so, good point.

anyway, as you said, local case is our main scenario for now. so it would be nice if streamJsonRpc supports custom cancellation logic.

...

our current LS, doesn't support cancellation in any way without custom cancellation support. so it doesn't matter whether things are connected through live share, remote vscode or etc. once it started processing a request, there is no way to bail out (cancelled).

@AArnott
Copy link
Member

AArnott commented Apr 2, 2020

We're out of time for VS 16.6 for new features and this would be a rather large one. Are you expecting/hoping for VS 16.7?

@heejaechang
Copy link
Author

16.7 sounds good? would that be possible?

...

so, I dug in a bit more on the remote issue. for vscode, it won't matter since it happens lower than the LSP layer. so everything works the same for LSP server's point of view.

for vs, live share requires custom code to start the lsp server and to communicate, so we should be good if VS language client exposes the custom cancellation mechanism.

@AArnott
Copy link
Member

AArnott commented Apr 4, 2020

I don't know if it will fit in 16.7. Tina would be the best one to ask. I might be able to fit this is myself, but I don't know at this point.

I don't understand how all the moving parts fit together that you mentioned, but it sounds like you still feel like streamjsonrpc having this feature is important to your scenario.

@heejaechang
Copy link
Author

heejaechang commented Apr 4, 2020

Ya. we need this to connect our python language server to VS. I will talk to Tina on VS LanguageClient part.

@heejaechang
Copy link
Author

any update? we (Pylance) is trying to use same server in VS as well now (which improve VS python quite a bit). we currently have working prototype but without cancellation.

@AArnott AArnott assigned AArnott and unassigned tinaschrepfer Sep 3, 2020
@AArnott
Copy link
Member

AArnott commented Sep 3, 2020

@heejaechang I suppose you only need the client side of a JSON-RPC connection to support this cancellation strategy, correct? No one would use StreamJsonRpc on the server side and expect the CancellationToken we give the server method to respond to some proprietary protocol, since it can support the proper LSP one, right?

@heejaechang
Copy link
Author

heejaechang commented Sep 3, 2020

@AArnott yep, we need client side support. that being said, not sure how much difference that would make.

it basically gets some kind of callback to call when cancellation is raised, and when the request is returned. and call the callback instead of sending "cancellation request".

since cancellation. as far as I know, raised only from the client-side, probably okay, but implementation wise, not sure whether that makes things any simpler?

@AArnott
Copy link
Member

AArnott commented Sep 3, 2020

It makes things simpler to only have to worry about client side, but it wasn't too much worse and makes me feel better about the extensibility point to allow customizing both ends. I'm finished with the code changes for this. I'm updating docs and will send a PR shortly.

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

Successfully merging a pull request may close this issue.

3 participants