Skip to content

Implement request cancellation #860

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Oct 26, 2023
Merged

Implement request cancellation #860

merged 5 commits into from
Oct 26, 2023

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Oct 9, 2023 •
edited
Loading

When receiving a CancellationNotification, we cancel the task that handles the request with that ID.

This will cause cancel_notification to be sent to sourcekitd or a CancellationNotification to be sent to clangd, which ultimately cancels the request.

rdar://117492860

@ahoppen ahoppen requested a review from benlangmuir as a code owner October 9, 2023 16:09
@ahoppen
Copy link
Member Author

ahoppen commented Oct 9, 2023

@swift-ci Please test

hamishknight
hamishknight approved these changes Oct 9, 2023
View reviewed changes
Copy link
Contributor

@hamishknight hamishknight left a comment

Choose a reason for hiding this comment

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

Nice!

@ahoppen ahoppen force-pushed the ahoppen/cancellation branch 2 times, most recently from 1ff9b9e to 75dbfd7 Compare October 9, 2023 22:00
@ahoppen
Copy link
Member Author

ahoppen commented Oct 9, 2023

@swift-ci Please test

hamishknight
hamishknight reviewed Oct 9, 2023
View reviewed changes
@ahoppen ahoppen force-pushed the ahoppen/cancellation branch from 75dbfd7 to 4238344 Compare October 9, 2023 23:37
@ahoppen
Copy link
Member Author

ahoppen commented Oct 9, 2023

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Oct 9, 2023

@swift-ci Please test Windows

1 similar comment
@ahoppen
Copy link
Member Author

ahoppen commented Oct 9, 2023

@swift-ci Please test Windows

@ahoppen
Copy link
Member Author

ahoppen commented Oct 10, 2023

This is currently blocked because we open the documents in sourcekitd semantically, not with syntax_only. We thus build an AST for the test file in testCancellation, which takes a couple of minutes, blocking sourckitd and makes us run into timeouts in later test cases that need semantic sourcekitd functionality. This should be fixed by changing open/edit to be syntactic only (kind of tracked by #882). I’ll let this

@ahoppen ahoppen marked this pull request as draft October 10, 2023 15:53
hamishknight
hamishknight reviewed Oct 11, 2023
View reviewed changes
hamishknight
hamishknight reviewed Oct 11, 2023
View reviewed changes
Sources/SKSupport/AsyncUtils.swift
/// cancelled, `cancel` is invoked with the handle that `operation` provided.
public func withCancellableCheckedThrowingContinuation<Handle, Result>(
_ operation: (_ continuation: CheckedContinuation<Result, any Error>) -> Handle,
cancel: (Handle) -> Void
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this probably ought to be marked @Sendable (I believe we'll warn under strict concurrency)

Copy link
Contributor

Choose a reason for hiding this comment

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

Talking of strict concurrency checking, are we planning on enabling that for this repo? I don't think it's a proper package option yet, but I think you can add .enableExperimentalFeature("StrictConcurrency") as a build setting

Copy link
Member Author

Choose a reason for hiding this comment

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

I was planning to try and enable strict concurrency checking once I don’t have any further async migrations in my backlog. Until I do that, I don’t think I care about sendable annotations too much.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough

@ahoppen ahoppen force-pushed the ahoppen/cancellation branch from a422359 to d7ade55 Compare October 12, 2023 16:57
@ahoppen ahoppen force-pushed the ahoppen/cancellation branch 2 times, most recently from a4d687f to 9b12ec2 Compare October 25, 2023 20:24
@ahoppen ahoppen marked this pull request as ready for review October 25, 2023 20:24
@ahoppen
Copy link
Member Author

ahoppen commented Oct 25, 2023

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Oct 25, 2023

Since we open documents as syntactic-only now, we can support cancellation. 🎉🏎️

bnbarham
bnbarham approved these changes Oct 25, 2023
View reviewed changes
Copy link
Contributor

@bnbarham bnbarham left a comment

Choose a reason for hiding this comment

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

🥳

@ahoppen
Copy link
Member Author

ahoppen commented Oct 26, 2023

@swift-ci Please test Windows

@ahoppen
Implement request cancellation
3d17cad 
When receiving a `CancellationNotification`, we cancel the task that handles the request with that ID.

This will cause `cancel_notification` to be sent to sourcekitd or a `CancellationNotification` to be sent to `clangd`, which ultimately cancels the request.

rdar://117492860
@ahoppen
Remove CancellationToken
71dfd48 
This is no longer needed because we handle cancellation on the `Task` level.
@ahoppen
Avoid a potential race condition in which a sourcekitd/clangd request…
b41f6af 
… wouldn't get cancelled
@ahoppen
Factor withCancellableCheckedThrowingContinuation into a separate f…
f7572c4 
…unction
@ahoppen
Update the document after cancelling the completion request to unbloc…
f29c97f 
…k sourcekitd
@ahoppen ahoppen force-pushed the ahoppen/cancellation branch from 9b12ec2 to f29c97f Compare October 26, 2023 18:04
@ahoppen
Copy link
Member Author

ahoppen commented Oct 26, 2023

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Oct 26, 2023

@swift-ci Please test Windows

1 similar comment
@ahoppen
Copy link
Member Author

ahoppen commented Oct 26, 2023

@swift-ci Please test Windows

@ahoppen ahoppen merged commit d043dc0 into swiftlang:main Oct 26, 2023
@ahoppen ahoppen deleted the ahoppen/cancellation branch October 26, 2023 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hamishknight hamishknight hamishknight approved these changes

@bnbarham bnbarham bnbarham approved these changes

@benlangmuir benlangmuir Awaiting requested review from benlangmuir

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ahoppen @bnbarham @hamishknight