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

Merged
merged 12 commits into from
Dec 9, 2024

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.

With this PR, internal error will occur whenever CheckAndThrow is called outside of Cancellable CE context.

  • Added test.
  • Runtime check can be disabled by setting DISABLE_CHECKANDTHROW_ASSERT env variable. I tested manually that it works.

Copy link
Contributor

github-actions bot commented Nov 20, 2024

❗ Release notes required


✅ Found changes and release notes in following paths:

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/9.0.200.md

@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
@psfinaki
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@psfinaki
Copy link
Member

@majocha do I get it right that the TODO in the description is already done?

@majocha
Copy link
Contributor Author

majocha commented Nov 25, 2024

@majocha do I get it right that the TODO in the description is already done?

Yes, forgot to edit this.

@T-Gro T-Gro self-assigned this Dec 3, 2024
@psfinaki psfinaki merged commit 9ab4e39 into dotnet:main Dec 9, 2024
33 checks passed
@majocha majocha deleted the checkandthrow branch December 9, 2024 12:04
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