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

Incremental build: catch exception during file checking #15906

Merged
merged 1 commit into from
Sep 1, 2023

Conversation

auduchinok
Copy link
Member

@auduchinok auduchinok commented Aug 31, 2023

Partially fixes #15905. Prevents the crash of the host process. Catches exception happening during a file analysis, preventing the host process crash. With this fix analysis of subsequent files don't work, but this prevents the crash and allows to fix the source code when it's the reason of the initial exception.

@auduchinok auduchinok requested a review from a team as a code owner August 31, 2023 13:18
@0101
Copy link
Contributor

0101 commented Aug 31, 2023

Hmm, wonder if we could somehow report the exception...

@auduchinok
Copy link
Member Author

auduchinok commented Aug 31, 2023

Ideally, the initial exception should be reported in #15907 when it's actually recovered and not rethrown. But, yeah, we should probably report it when it gets here somehow. I'd rather keep it out of scope of this PR, so we only fix the crash here, and it's easier to cherry-pick or revert it independently of other changes.

@psfinaki
Copy link
Member

Any way to test this?

@auduchinok
Copy link
Member Author

Any way to test this?

We could try to simulate the behaviour from #15905, but #15907 should fix the exception that we would be trying to catch here. I don't have another idea of a test from the top of my head, unfortunately.

@ForNeVeR
Copy link
Contributor

An alternate solution I'd propose would be to ban Async.Start completely from the compiler and the tooling code. This function is too dangerous for anyone to use.

(We have a very few of its existing usages anyway.)

Just replace it with Async.StartAsTask, and add _ = Async.StartAsTask or ignore <| as required to avoid warnings (this will also additionally signify that you are starting a "fire and forget"-kind job). Tasks have good and well-known behavior w.r.t. uncaught exceptions, while this particular aspect of behavior of Async.Start (i.e. its tendency to crash the runtime) is not widely known and occasionally hits people.

@vzarytovskii
Copy link
Member

An alternate solution I'd propose would be to ban Async.Start completely from the compiler and the tooling code. This function is too dangerous for anyone to use.

(We have a very few of its existing usages anyway.)

Just replace it with Async.StartAsTask, and add _ = Async.StartAsTask or ignore <| as required to avoid warnings (this will also additionally signify that you are starting a "fire and forget"-kind job). Tasks have good and well-known behavior w.r.t. uncaught exceptions, while this particular aspect of behavior of Async.Start (i.e. its tendency to crash the runtime) is not widely known and occasionally hits people.

We want to eventually move to cancellable tasks outside of public APIs, as they shown to be quite effective in VS extension, which should solve the exceptions problem.

@ForNeVeR
Copy link
Contributor

That also sounds like a good solution 👍

@T-Gro T-Gro merged commit 23b87bc into dotnet:main Sep 1, 2023
@auduchinok auduchinok deleted the incrementalBuild-catch branch September 1, 2023 08:29
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.

Exception during type checking may lead to FCS host process crash
6 participants