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

Log before unregistering Fixes #9455 #9571

Merged
merged 4 commits into from
Jan 16, 2024

Conversation

Forgind
Copy link
Member

@Forgind Forgind commented Dec 21, 2023

Fixes #9455

Context

If we crash very early—like in the midst of setting up the ProjectLoggingContext—we have all the machinery set up to log information like that the build started, but then we unregister the relevant handlers before we actually log that we'd failed.

Changes Made

Do not unregister handlers until shutdown

Testing

I tested with the repro from #9455, and it now looks like this:

MSBUILD : error MSB4017: The build stopped unexpectedly because of an unexpected logger failure.
MSBUILD : error MSB4017:     Exception of type 'System.Exception' was thrown.
MSBUILD : error MSB4017:        at Microsoft.Build.BackEnd.Logging.ParallelConsoleLogger.ProjectStartedHandler(Object s
ender, ProjectStartedEventArgs e) in C:\GitHub\msbuild\src\Build\Logging\ParallelLogger\ParallelConsoleLogger.cs:line 5
86
MSBUILD : error MSB4017:    at Microsoft.Build.Evaluation.ProjectCollection.ReusableLogger.ProjectStartedHandler(Object
 sender, ProjectStartedEventArgs e) in C:\GitHub\msbuild\src\Build\Definition\ProjectCollection.cs:line 2342
MSBUILD : error MSB4017:    at Microsoft.Build.BackEnd.Logging.EventSourceSink.RaiseProjectStartedEvent(Object sender,
ProjectStartedEventArgs buildEvent) in C:\GitHub\msbuild\src\Build\BackEnd\Components\Logging\EventSourceSink.cs:line 5
33

(with a full stack trace).

Notice that it finds the real exception—just an ordinary System.Exception with no message in this case.

Notes

I went directly against the advice of a comment to make this change. I tried to see what motivated that comment, but it came with the initial commit along with all the code next to it. Regardless, it does not make me feel good to do so. I don't understand why the comment should be true, but please do read it and let me know if this is a horrible idea.

I also intentionally reused MSB4017 because this feels like the same exception, and I didn't want to confuse people. I just added some extra formatting around it. Like with going against the comment, I can find a new number if reviewers don't like that idea.

Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Unexpected order of messages seems less harm then losing the exception info - so the change looks good to me.

I'm just wondering whether the rethrowing info wouldn't be helpful as well

@JanKrivanek JanKrivanek merged commit 98586a2 into dotnet:main Jan 16, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: error MSB4017 is not shown consistently when the internal logger exception occurs.
3 participants