Skip to content

added file-based cancellation support to JSON-RPC#587

Merged
dbaeumer merged 27 commits intomicrosoft:masterfrom
heejaechang:fileBasedCancellation
Mar 23, 2020
Merged

added file-based cancellation support to JSON-RPC#587
dbaeumer merged 27 commits intomicrosoft:masterfrom
heejaechang:fileBasedCancellation

Conversation

@heejaechang
Copy link
Contributor

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)

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)
@heejaechang
Copy link
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.

…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.
@heejaechang
Copy link
Contributor Author

@dbaeumer can you take a look again?

@heejaechang heejaechang requested a review from dbaeumer March 16, 2020 16:34
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.
@heejaechang heejaechang force-pushed the fileBasedCancellation branch from 32b15da to 75f5228 Compare March 23, 2020 09:18
Copy link
Member

@dbaeumer dbaeumer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like it very much!. Only some minor comments.

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 this pull request may close these issues.

2 participants