Random cleanup #7173

Forgind · 2021-12-29T19:33:54Z

I was doing this as part of the TaskHost change. In most cases, it made it easier to understand why tests were failing. Then I went a little further on a couple points, most notably exceptions.

I think the commits are pretty clean. Most are also quite short.

rainersigwald · 2022-01-05T17:26:12Z

src/Build.UnitTests/EscapingInProjects_Tests.cs


-                bool success = project.Build(logger);
-                Assert.True(success); // "Build failed.  See test output (Attachments in Azure Pipelines) for details"
+                project.Build(logger).ShouldBeTrue(); // "Build failed.  See test output (Attachments in Azure Pipelines) for details"


While we're here might as well bring back the message:

Suggested change

project.Build(logger).ShouldBeTrue(); // "Build failed. See test output (Attachments in Azure Pipelines) for details"

project.Build(logger).ShouldBeTrue("Build failed. See test output (Attachments in Azure Pipelines) for details");

rainersigwald · 2022-01-05T17:28:22Z

src/Utilities/MuxLogger.cs

                            // first unregister all loggers, since other loggers may receive remaining events in unexpected orderings
                            // if a fellow logger is throwing in an event handler.
                            UnregisterAllEventHandlers();
-


I personally prefer the whitespace here; do you feel strongly about it?

Only a slight preference, but I do have a stronger preference for either whitespace or no whitespace rather than mixed—the catch (LoggerException) right above this has no space, for instance.

rainersigwald · 2022-01-05T21:00:50Z

src/Shared/BuildEnvironmentHelper.cs

-
-                    throw;
+                    // Throw the error that caused the TypeInitializationException, likely InvalidOperationException.
+                    throw e.InnerException ?? e;


I think this is actually different, and you should not make this change: throw e; and throw; have different results, because throw e; throws from this stack location, while throw; rethrows the existing exception with its own stack location.

rainersigwald · 2022-01-05T21:03:40Z

src/Build/Instance/TaskRegistry.cs

                                return false;
                            }
-                            catch (Exception e) // Catching Exception, but rethrowing unless it's a well-known exception.
+                            catch (Exception e) when (!ExceptionHandling.IsCriticalException(e)) // Catching Exception, but rethrowing unless it's a well-known exception.


In other cases you dropped the comment. Why not here?

rainersigwald · 2022-01-05T21:03:41Z

src/Build/Instance/TaskRegistry.cs

                                return false;
                            }
-                            catch (Exception e) // Catching Exception, but rethrowing unless it's a well-known exception.
+                            catch (Exception e) when (!ExceptionHandling.IsCriticalException(e)) // Catching Exception, but rethrowing unless it's a well-known exception.


In other cases you dropped the comment. Why not here?

rainersigwald · 2022-01-05T21:08:50Z

src/Build/BackEnd/Shared/BuildRequestConfiguration.cs

                }
            }
-            catch (Exception e)
+            catch (DirectoryNotFoundException e)


Suggested change

catch (DirectoryNotFoundException e)

catch (DirectoryNotFoundException e) when (e is DirectoryNotFoundException || e is UnauthorizedAccessException)

instead?

rainersigwald · 2022-01-05T21:25:27Z

src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs

-                thrownException = ex;
-            }
-            catch (Exception ex)
+            catch (Exception ex) // LoggerException is a polite logger failure. InternalLoggerException is an arbitrary exception. Handle them the same.


I don't understand the "handle them the same" part of this comment. Maybe just delete the comment?

rainersigwald · 2022-01-05T21:27:14Z

src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs

            {
                _taskLoggingContext.LogWarningFromText(null, warningCode, helpKeyword, new BuildEventFileInfo(_taskLocation), message);
            }
-            catch (InternalErrorException) // BUGBUG, should never catch this


Do you think it's ok to catch this now? Or should we leave the comment that we shouldn't be but are "temporarily"?

This didn't really sound like it was a bug to me. Sure, it would be better if we never threw that exception, and we could accomplish something like that by having something to check whether we were cancelled before throwing, but having a catch feels equivalent to me as long as there aren't other ways internal exceptions can come through here. Perhaps it would be slightly better if we had catch (InternalException) when (!_taskLoggingContext.IsValid && weWerentCancelled), but for a try/catch around a single logging statement, that feels like overkill.

Forgind · 2022-01-10T16:57:33Z

src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs

                    taskCleanedUp = _requestTask.Wait(BuildParameters.RequestBuilderShutdownTimeout);
                }
-                catch (AggregateException e)
+                catch (AggregateException e) when (e.Flatten().InnerExceptions.All(ex => ex is TaskCanceledException || ex is OperationCanceledException))


Make a named function here.

Forgind · 2022-01-10T19:02:56Z

src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs

            }
-            catch (Exception e)
+            // If a logger has failed, abort immediately. This is the polite LoggerException.
+            // InternalLoggerException is an arbitrary logger exception.


Leaving this case of 3+ clauses in because I'd added a comment previously.

Forgind · 2022-01-10T19:04:00Z

src/Build/Logging/LoggerDescription.cs

            }
-            catch (Exception e /* Wrap all other exceptions in a more meaningful exception*/)
+            catch (Exception e) // Wrap other exceptions in a more meaningful exception. LoggerException and InternalLoggerException are already meaningful.
+            when (!(e is LoggerException /* Polite logger Failure*/ || e is InternalLoggerException /* LoggerClass not found*/ || ExceptionHandling.IsCriticalException(e)))


Forgind · 2022-01-10T19:04:37Z

src/Build/Logging/LoggerDescription.cs

+                // Logger failed politely during construction. In order to preserve
+                // the stack trace at which the error occurred we wrap the original
+                // exception instead of throwing.
+                throw new LoggerException(le.Message, le, le.ErrorCode, le.HelpKeyword);


Why not just throw le? (I saw the comment, but I don't see why making a new exception would preserve the stack trace, whereas throwing the exception that had the stack trace wouldn't.)

rokonec

I have carefully reviewed it.
Please consider all my comments as an optional feedback ideas.
Good job.

src/Build.UnitTests/EscapingInProjects_Tests.cs

src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs

src/MSBuild/XMake.cs

src/Shared/LogMessagePacketBase.cs

src/Shared/UnitTests/ObjectModelHelpers.cs

Forgind added 7 commits December 29, 2021 09:59

Use Shouldly

ece10a6

Delete ToolsVersion and xmlns

d3d7f3c

new() without type

dcf30e5

Use $ to format string literals

f4b6729

Clean up exceptions

6eb5b48

Inline variable declaration

67b91d9

=> for simple methods

e66628f

rainersigwald reviewed Jan 5, 2022

View reviewed changes

rainersigwald approved these changes Jan 5, 2022

View reviewed changes

Forgind added 2 commits January 6, 2022 11:01

Add space

c669c98

PR comments

cd50fe3

Forgind commented Jan 10, 2022

View reviewed changes

Use named functions to clarify meaning of exceptions

d609930

Forgind commented Jan 10, 2022

View reviewed changes

rokonec approved these changes Jan 10, 2022

View reviewed changes

Use ContainsKey

59b556e

Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Jan 11, 2022

Forgind merged commit c54088d into dotnet:main Jan 11, 2022

Forgind deleted the clean-taskhost branch January 11, 2022 15:21

This was referenced Jul 29, 2025

Bump Azure.Storage.Blobs and 34 others rajkumar-rangaraj/opentelemetry-dotnet-instrumentation#1000

Closed

Bump Azure.Storage.Blobs and 37 others rajkumar-rangaraj/opentelemetry-dotnet-instrumentation#1003

Closed

dependabot bot mentioned this pull request Aug 15, 2025

Bump Microsoft.Build from 15.9.20 to 17.11.31 rajkumar-rangaraj/opentelemetry-dotnet-instrumentation#1036

Closed

dependabot bot mentioned this pull request Oct 15, 2025

Bump Microsoft.Build from 15.9.20 to 17.11.48 rajkumar-rangaraj/opentelemetry-dotnet-instrumentation#1085

Open

	project.Build(logger).ShouldBeTrue(); // "Build failed. See test output (Attachments in Azure Pipelines) for details"
	project.Build(logger).ShouldBeTrue("Build failed. See test output (Attachments in Azure Pipelines) for details");

	catch (DirectoryNotFoundException e)
	catch (DirectoryNotFoundException e) when (e is DirectoryNotFoundException \|\| e is UnauthorizedAccessException)

Uh oh!

Random cleanup #7173

Random cleanup #7173

Uh oh!

Conversation

Forgind commented Dec 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Forgind Jan 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Forgind Jan 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rokonec left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Forgind Jan 10, 2022 •

edited

Loading

Forgind Jan 10, 2022 •

edited

Loading