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

Make sure CheckAndThrow is invoked only within ambient Cancellable context #18037

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

majocha
Copy link
Contributor

@majocha majocha commented Nov 20, 2024

See: #18002 (comment)

Basically, when Cancelable.CheckAndThrow() throws the exception, we don't want the exception to leak.
It should be caught inside Cancellable.run here:

let inline run (ct: CancellationToken) (Cancellable oper) =
if ct.IsCancellationRequested then
ValueOrCancelled.Cancelled(OperationCanceledException ct)
else
try
use _ = Cancellable.UsingToken(ct)
oper ct
with :? OperationCanceledException as e ->
ValueOrCancelled.Cancelled(OperationCanceledException e.CancellationToken)

The idea of CheckAndThrow is that it is invoked from code that is unaware of Cancellable CE somewhere deeper on the callstack.
This PR checks the static AsyncLocal<CancellationToken voption> token. If it is default VNone, we know the invocation is outside of Cancellable. If there is a token present, we know Cancellable.run deeper on the stack has set it and we're good.

TODO: fix ModuleReaderCancellationTests.

cc @auduchinok

Copy link
Contributor

❗ Release notes required

@majocha,

Caution

No release notes found for the changed paths (see table below).

Please make sure to add an entry with an informative description of the change as well as link to this pull request, issue and language suggestion if applicable. Release notes for this repository are based on Keep A Changelog format.

The following format is recommended for this repository:

* <Informative description>. ([PR #XXXXX](https://github.com/dotnet/fsharp/pull/XXXXX))

See examples in the files, listed in the table below or in th full documentation at https://fsharp.github.io/fsharp-compiler-docs/release-notes/About.html.

If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request.

You can open this PR in browser to add release notes: open in github.dev

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/9.0.200.md No release notes found or release notes format is not correct

@@ -14,6 +15,7 @@ type FSharpXunitFramework(sink: IMessageSink) =
// right at the start of the test run is here in the constructor.
// This gets executed once per test assembly.
MessageSink.sinkWriter |> ignore
Cancellable.EnsureCheckAndThrowInvokedWithAmbientCancellable()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This just installs the check globally for the whole test run.

@auduchinok
Copy link
Member

Maybe it'd make sense to have this assert not in the tests only? I can imagine some FCS APIs that may trigger analysis and that don't take a cancellation token. If there're no tests for such APIs, it's going to be very difficult to uncover these, as they'd only manifest themself when cancelled at some unfortunate moment. The sooner we catch all these exceptions, the better.

Maybe we could have some global property or other way to control it. With this way we'd enable these asserts in the internal and EAP Rider builds, but could turn it off just in case in the release ones, at least for the time being.

@majocha
Copy link
Contributor Author

majocha commented Nov 20, 2024

TBH my initial thought was to just put a hard failwith where the assert is. It would surface as a internal error (see the failing tests) if CheckAndThrow is misused anywhere. This would be immediately discoverable.

@majocha
Copy link
Contributor Author

majocha commented Nov 21, 2024

Added DISABLE_CHECKANDTHROW_ASSERT env variable that can be set in case of emergencies.

@majocha majocha marked this pull request as ready for review November 22, 2024 07:47
@majocha majocha requested a review from a team as a code owner November 22, 2024 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: New
Development

Successfully merging this pull request may close these issues.

2 participants