Skip to content

Diagnostics logger: remove exception rethrow during recovery #15907

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

Merged
merged 6 commits into from
Sep 5, 2023

Conversation

auduchinok
Copy link
Member

Partially fixes #15905. Removes rethrowing exceptions in DiagnosticsLoggerExtensions.ErrorRecovery, which was making the error recovery non-existent for many exceptions other than InternalError. System exceptions like ArgumentException or NullReferenceException were never recovered because of this call, breaking analysis for the whole file in many cases.

With this change the analysis recovery properly continues but the internal error isn't reported anymore, I'm still looking into it.

@psfinaki
Copy link
Member

Watsonable. Really? :D

Have you found the reason why this was put there initially?

@auduchinok
Copy link
Member Author

Have you found the reason why this was put there initially?

I goes beyond the initial Git commit in this repo, so can't say for sure. Our guess was the authors wanted the process to crash to analyse the crash. We never want an IDE crash because of an error during analysis. 🙂

@vzarytovskii
Copy link
Member

Have you found the reason why this was put there initially?

I goes beyond the initial Git commit in this repo, so can't say for sure. Our guess was the authors wanted the process to crash to analyse the crash. We never want an IDE crash because of an error during analysis. 🙂

That's exactly the reason. The idea is - we want to crash if we can't recover/repair it. In VS it won't crash the whole IDE, but only extension.

@auduchinok
Copy link
Member Author

That's exactly the reason. The idea is - we want to crash if we can't recover/repair it.

The problem is it's called in ErrorRecovery, effectively breaking the recovery. Analysis continues just fine if this code is removed, both in the current file and in subsequent ones, because ErrorRecovery is called in places where we can actually continue.

@ForNeVeR
Copy link
Contributor

In VS it won't crash the whole IDE, but only extension.

I think we observed the whole VS process crashing because of an uncaught exception (see #15906 for the fix of that).

@vzarytovskii
Copy link
Member

That's exactly the reason. The idea is - we want to crash if we can't recover/repair it.

The problem is it's called in ErrorRecovery, effectively breaking the recovery. Analysis continues just fine if this code is removed, both in the current file and in subsequent ones, because ErrorRecovery is called in places where we can actually continue.

What I meant by that, if we hit something truly irrecoverable. Like, we can't read pickled data, or some critical issue, we want to crash. I'm not sure why is it in the diagnostic logged.

@auduchinok
Copy link
Member Author

Like, we can't read pickled data, or some critical issue, we want to crash. I'm not sure why is it in the diagnostic logged.

I think that even in this situation we should always try to continue the work and report the error via diagnostic. Currently this PR hides the error, but I'm going to improve it before the merge.

@ForNeVeR
Copy link
Contributor

The current behavior (just let the exception to be rethrown several times in hope somebody/or even an attached debugger will catch it later) looks unusual/dubious to me. And it provably doesn't always work.

Doesn't VS have any better error reporting facilities, which would allow reporting an exception?

Since we know that exceptions of this kind are normally pretty rare (well, VS doesn't crash for you every day, is it?) due to very good quality of the compiler code (I am not ironic, I really think so), perhaps we could just follow the existing error reporting guidelines and show exception info somewhere in the status bar or whatnot?

Or maybe there's some internal error processing pipeline wired with IDE telemetry that will automatically report the collected errors when allowed by the user?

@vzarytovskii
Copy link
Member

Like, we can't read pickled data, or some critical issue, we want to crash. I'm not sure why is it in the diagnostic logged.

I think that even in this situation we should always try to continue the work and report the error via diagnostic. Currently this PR hides the error, but I'm going to improve it before the merge.

Controlled report is preferable, but it doesn't allow to collect nearly as much info as watson allows. Also, we can't really collect much from the compiler itself.

Doesn't VS have any better error reporting facilities, which would allow reporting an exception?

Rather limited, I whink the only way to collect full watson dump was to crash sadly.

Or maybe there's some internal error processing pipeline wired with IDE telemetry that will automatically report the collected errors when allowed by the user?

Yeah, that would be nice, but will require significant rewrite and a bunch of GDPR-related approvals to make sure we don't collect something we shouldn't.

@auduchinok
Copy link
Member Author

auduchinok commented Aug 31, 2023

Controlled report is preferable, but it doesn't allow to collect nearly as much info as watson allows. Also, we can't really collect much from the compiler itself.

What should we do here? Prior to this PR, this code would just break error recovery in the type checker in many cases, also breaking analysis of subsequent files. It all happened in cases where we expected errors to be recoverable, because the only place where this rethrow is happening is during error recovery.

What short-term options I see:

  • Log exception stacktrace via Debug.Assert instead of throwing again
    That would usually be enough for us to investigate the issue and may significantly improve user experience when an exception happens.

  • Add a setting to FCS, to either rethrow (i.e. break the recovery) or to do the logging, as suggested above
    This would allow to crash VS process if that's actually needed for investigations and a stacktrace logging isn't enough

@auduchinok
Copy link
Member Author

I've rechecked this fix without #15909 applied, and it turns out the errors aren't hidden and are properly reported via a diagnostic:

Screenshot 2023-09-01 at 10 14 18

Please note, that the rest of the analysis has also managed to finish, as can be seen from the highlighted identifiers.

With this info, I'd vote for a better exception logging (so the stacktraces are available somewhere) and not rethrowing the exception, at least by default. @vzarytovskii Please let me know how I should proceed here.

@vzarytovskii
Copy link
Member

vzarytovskii commented Sep 1, 2023

I've rechecked this fix without #15909 applied, and it turns out the errors aren't hidden and are properly reported via a diagnostic:

Screenshot 2023-09-01 at 10 14 18

Please note, that the rest of the analysis has also managed to finish, as can be seen from the highlighted identifiers.

With this info, I'd vote for a better exception logging (so the stacktraces are available somewhere) and not rethrowing the exception, at least by default. @vzarytovskii Please let me know how I should proceed here.

Yeah, not rethrowing is the right thing to do.

@auduchinok auduchinok force-pushed the diagnostics-watsonable branch from 0d23553 to 017841c Compare September 4, 2023 11:57
@auduchinok
Copy link
Member Author

auduchinok commented Sep 4, 2023

I've added InternalException for wrapping a system exceptions and capturing their stacktraces. These exceptions get logged via Debug.Trace and show tooltip with the 'internal error' and exception message in both Release/Debug.

I've checked it on ArgumentException and NullReferenceException types in Rider, and it worked good. We can tweak this in future PRs if we think that it's suboptimal in some scenarios/tools.

@T-Gro T-Gro merged commit 60d2dbd into dotnet:main Sep 5, 2023
@auduchinok auduchinok deleted the diagnostics-watsonable branch September 5, 2023 10:22
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