added file-based cancellation support to JSON-RPC#587
Merged
dbaeumer merged 27 commits intomicrosoft:masterfrom Mar 23, 2020
Merged
added file-based cancellation support to JSON-RPC#587dbaeumer merged 27 commits intomicrosoft:masterfrom
dbaeumer merged 27 commits intomicrosoft:masterfrom
Conversation
Existing cancellation mechanism works fine for code that embraces async/await, but it doesn't work for synchronous code path since it requires code to yield to the event loop to handle cancellation notification. This new mode is for those people who want to use cancellation with synchronous code path at the expense of file I/O. The existing cancellation mechanism is still there and works as it used to be, so if the event loop gets to the cancellation check first, it will still work. if two ends of JSON-RPC uses different cancellation mode, it will fall back to existing cancellation mode (cancel notification)
Contributor
Author
|
tagging @dbaeumer can you take a look? this only include changes in jsonrpc, if you are okay with this change, I will plumbing it out to LanguageClient with new options there. |
heejaechang
commented
Mar 12, 2020
… in the protocol repo and added cancellation tests
heejaechang
commented
Mar 12, 2020
heejaechang
commented
Mar 12, 2020
heejaechang
commented
Mar 12, 2020
heejaechang
commented
Mar 12, 2020
…ssible. This is a best-effort approach, so it doesn't guarantee all files will be deleted at the end. For example, if the client-side tries to raise cancellation right after the server responded, but before the client-side responded to the reply, then it could create a cancellation file that no one is looking for. Another case would be if cancellation is raised, but no one in the server-side checked the cancellation, then the server wouldn't know that cancellation is raised, and the cancellation file created won't be deleted. But the owner of the JSON-RPC connection owns the cancellation file folder, so he should take care of resources left at the end of the session.
dbaeumer
requested changes
Mar 13, 2020
…ep backward compatibility
Contributor
Author
|
@dbaeumer can you take a look again? |
dbaeumer
reviewed
Mar 17, 2020
… cancellation.fileBased.ts
…ow, but once code is changed to use strategy, it will get cleaned up.
heejaechang
commented
Mar 19, 2020
heejaechang
commented
Mar 19, 2020
dbaeumer
requested changes
Mar 19, 2020
First, I forgot to put clean up at the end of a test. Second, I found an issue where the event dispose method throws an exception if it is called after Emitter is disposed. This can now happen for CancellationTokenSource since messageConnection holds onto disposable from event subscription to call clean up after request is completed (succeeded or not). But most of the time, that is after cancellation is called for the source, which disposes the Emitter. so we need to handle this case.
dbaeumer
reviewed
Mar 23, 2020
dbaeumer
reviewed
Mar 23, 2020
dbaeumer
reviewed
Mar 23, 2020
dbaeumer
reviewed
Mar 23, 2020
dbaeumer
reviewed
Mar 23, 2020
dbaeumer
reviewed
Mar 23, 2020
dbaeumer
reviewed
Mar 23, 2020
dbaeumer
reviewed
Mar 23, 2020
32b15da to
75f5228
Compare
dbaeumer
requested changes
Mar 23, 2020
Member
dbaeumer
left a comment
There was a problem hiding this comment.
Like it very much!. Only some minor comments.
dbaeumer
approved these changes
Mar 23, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Existing cancellation mechanism works fine for code that embraces async/await, but it doesn't work for synchronous code path since it requires code to yield to the event loop to handle cancellation notification.
This new mode is for those people who want to use cancellation with synchronous code path at the expense of file I/O.
The existing cancellation mechanism is still there and works as it used to be, so if the event loop gets to the cancellation check first, it will still work.
if two ends of JSON-RPC uses different cancellation mode, it will fall back to existing cancellation mode (cancel notification)