-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Support cancellation requests #18627
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
Conversation
|
|
dhruvmanila
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I haven't looked very closely at the code but it all looks almost same as the one in ty_server so I've provided a couple of recommendations to de-duplicate if possible. This doesn't need to happen in this PR and we can also open a "help-wanted" issue for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks exactly the same as the one in the ty_server, can we re-use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also looks the same as the one in ty_server except for the retry method, maybe this could be re-used as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's almost the same with the exception that it doesn't have a retry function.
It's not a "difficult" problem but there is no good place for the files that are shared between ruff and the ty server. And Client depends on Session, which makes the reuse a bit of a challenge.
Summary
This PR adds support for the LSP's cancellation requests.
This is mainly a backport of the following two ty PRs:
Unlike ty, this PR doesn't add support for retries. Retries aren't necessary because
Ruff doesn't have to handle Salsa cancellations
Requester,ResponderandNotifierand unifies them under a singleClientAPI. I found them more confusing than helpful, and I don't see any reason why we should restrict background tasks from sending requests.ClientSender: This is mostly a fallout of the above.IOThreadsout ofConnection: This removes the entire complexity around weak references because scoping rules now ensure thatConnection(and its senders) are dropped before we join the thread.MESSENGERand instead require callers to explicitly pass aClient. This helped findshow_messagecall sites whereMESSENGERwasn't initialized. It also removes global state, which is always goodSessionthat background tasks don't have. This approach is the same as r-a's.Test Plan
Started VS code with the new ruff server.