-
Notifications
You must be signed in to change notification settings - Fork 794
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
Conversation
❗ Release notes required
|
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. |
TBH my initial thought was to just put a hard |
Added DISABLE_CHECKANDTHROW_ASSERT env variable that can be set in case of emergencies. |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
@majocha do I get it right that the TODO in the description is already done? |
Yes, forgot to edit this. |
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:fsharp/src/Compiler/Utilities/Cancellable.fs
Lines 44 to 52 in e44ae1d
The idea of
CheckAndThrow
is that it is invoked from code that is unaware ofCancellable
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 knowCancellable.run
deeper on the stack has set it and we're good.With this PR, internal error will occur whenever
CheckAndThrow
is called outside of Cancellable CE context.