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

Convert OCE to empty value on certain VS operations #17906

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

Conversation

0101
Copy link
Contributor

@0101 0101 commented Oct 22, 2024

Description

There are OperationCancelledExceptions in telemetry originating from FSharp project options not found. in AddSemanticClassificationsAsync and GetBlockStructureAsync operations.

There's not much we can do without options but maybe we can just return empty results instead of propagating OCE further up.

One reason we don't have options is that when project is loading and an F# file is already open VS asks as for semantic classification, block structure (and some other things) using a MiscellaneousFiles workspace. There could potentially be other cases.

This proposed change converts all OCEs in those operations to empty results, unless cancellation has been requested by the caller through the CT they passed us.

Copy link
Contributor

github-actions bot commented Oct 22, 2024

⚠️ Release notes required, but author opted out

Warning

Author opted out of release notes, check is disabled for this pull request.
cc @dotnet/fsharp-team-msft

cancellableTask {
try
return! ctask
with :? OperationCanceledException ->
Copy link
Member

Choose a reason for hiding this comment

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

We should only do that if the token causing the OCE is a different one from the one our cancellable uses.
Or, maybe a lot more explicit - out code failing on project options would throw any other exception, which will be explicitly caught here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should only do that if the token causing the OCE is a different one from the one our cancellable uses.

What token is used when we throw from our cancellable? Also how can we tell who caused it?

Or, maybe a lot more explicit - out code failing on project options would throw any other exception, which will be explicitly caught here.

The downside of that is that we would have to start catching it in a lot of places.


Also, what actually happens when this is canceled for another reason? Since the OCE from here goes to Watson no one is relying on it in any way. Assuming the caller handles it somehow when they initiate the cancellation, they shouldn't have a problem with us returning an empty result (which will in that case probably just be thrown away).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, we could check that the (CancellationToken we get called with).IsCancellationRequested = false which seems to be the case when we just throw an OCE.

Copy link
Member

Choose a reason for hiding this comment

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

OCE exception has a field with the token that caused it. Comparison of the two tokens should be possible.

Copy link
Member

Choose a reason for hiding this comment

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

Btw. the fact that we are (sometimes) reporting OCE to Watson is a mistake caused by general catch handle on our end, we should not be doing that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw. the fact that we are (sometimes) reporting OCE to Watson is a mistake caused by general catch handle on our end, we should not be doing that.

How is that? These get called through MEF. We're not catching anything. Or are we hooking into this mechanism somewhere indirectly?

Copy link
Member

Choose a reason for hiding this comment

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

At some places, we have a universal try-with catching all exceptions, and doing our own telemetry reporting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah we should deal with those. But it's not the case here.

@0101 0101 linked an issue Oct 22, 2024 that may be closed by this pull request
@0101 0101 added the NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes label Oct 22, 2024
@0101 0101 marked this pull request as ready for review October 22, 2024 14:44
@0101 0101 requested a review from a team as a code owner October 22, 2024 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Distracting and not helpful error toast in VS on OperationCanceled
3 participants