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

Add Cancellable.CheckAndThrow #16137

Merged
merged 12 commits into from
Oct 24, 2023

Conversation

auduchinok
Copy link
Member

@auduchinok auduchinok commented Oct 18, 2023

This PR adds a way to check whether a cancellation is requested in situations where it's not feasible or desirable to explicitly pass a cancellation token or to wrap everything into cancellable CE expressions. It uses the same approach as the one used for logging diagnostics and keeping track of the current analysis phase: setting a thread local field and returning the previous state at the end via an implicit Dispose call.

There're currently two main use cases where this is needed:

  • long running analysis, too deep inside expressions analysis to use cancellable or pass a token (think about pattern match compilation or computation expression analysis). We want to be able to cancel the work timely if a cancellation was requested by the FCS client; parts of cancellable computations between checks are too coarse for it
  • when control reaches shims like file system or assembly reader, we want to prevent access to content model from a cancelled request to ensure we don't run analysis on outdated or inconsistent state

This adds a general and very simple to use method to cancel work from any depth of the analysis, and we can start adding CheckAndThrow checks in places where analysis tends to take more time, according to snapshots. I've added one such check to the pattern match analysis.

We can also add cancellation tokens to more of FSharpCheckFileResults APIs in future, so IDE features like code completion could also be improved by cancelling earlier if needed.

To make it easier to review, I've split the changes into multiple commits, and the most interesting part of the changes is placed into 'Allow checking cancellation inside analysis' commit.

This is work in progress, I'll be happy to get feedback on how this could be improved.

@auduchinok auduchinok requested a review from a team as a code owner October 18, 2023 14:15
@vzarytovskii vzarytovskii marked this pull request as draft October 18, 2023 14:19
@auduchinok
Copy link
Member Author

The next step I want to do is to make reading metadata in Import ready for OperationCanceledException. I've been looking into it today, and it seems there may be several changes that, apart from other things, would make that code more thread-safe. I think, I'll prefer to do that in a separate PR, to keep the changes unrelated, so this one is ready for review now.

@auduchinok auduchinok marked this pull request as ready for review October 18, 2023 16:38
@vzarytovskii
Copy link
Member

vzarytovskii commented Oct 18, 2023

We've discussed the approach with Eugene already. Since we're doing it now for diagnostics and logging, I think it's fine until we have better solution. Passing ct everywhere is an option too, but it can be tedious in some of the stacks.

One thing I'm slightly concerned about is our stack guard. @auduchinok it might get in the way of threadstatic, could you please try and test its behaviour?
Worst case scenario it's not gonna be cancelled.

@auduchinok
Copy link
Member Author

One thing I'm slightly concerned about is our stack guard. @auduchinok it might get in the way of threadstatic, could you please try and test its behaviour?
Worst case scenario it's not gonna be cancelled.

Thanks for the pointer! I've already covered it in https://github.com/dotnet/fsharp/pull/16137/files#diff-fe45c1d1cd0e4146b86e4222f395379a6b30118164100e90f5baefa54eb6bf15R866 the same way as logging and diagnostics, so I think it should be fine, but I haven't tested it specifically.

@vzarytovskii vzarytovskii enabled auto-merge (squash) October 24, 2023 18:34
@vzarytovskii vzarytovskii enabled auto-merge (squash) October 24, 2023 18:34
@vzarytovskii vzarytovskii merged commit 977d853 into dotnet:main Oct 24, 2023
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants