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.

Sign up for GitHub

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

Don't hang builds on BuildManager internal errors #5917

Merged
merged 5 commits into from
Dec 4, 2020
Merged

Don't hang builds on BuildManager internal errors #5917

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ab339d7
Add unit test
ladipro Nov 26, 2020
4dd56eb
Complete submissions on thread exceptions
ladipro Nov 26, 2020
564dd95
PR feedback: Named parameter and cleaner test project file
ladipro Dec 3, 2020
04273cc
Update src/Build.UnitTests/BackEnd/BuildManager_Tests.cs
ladipro Dec 3, 2020
fb4f36c
Fix test flakiness - do not run the new test under MSBUILDINPROCENVCHECK
ladipro Dec 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 35 additions & 1 deletion src/Build.UnitTests/BackEnd/BuildManager_Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,11 @@ public class BuildManager_Tests : IDisposable
private readonly TestEnvironment _env;
private readonly ITestOutputHelper _output;

/// <summary>
/// The transient state corresponding to setting MSBUILDINPROCENVCHECK to 1.
/// </summary>
private readonly TransientTestState _inProcEnvCheckTransientEnvironmentVariable;

/// <summary>
/// SetUp
/// </summary>
Expand All @@ -77,7 +82,7 @@ public BuildManager_Tests(ITestOutputHelper output)
_projectCollection = new ProjectCollection();

_env = TestEnvironment.Create(output);
_env.SetEnvironmentVariable("MSBUILDINPROCENVCHECK", "1");
_inProcEnvCheckTransientEnvironmentVariable = _env.SetEnvironmentVariable("MSBUILDINPROCENVCHECK", "1");
}

/// <summary>
Expand Down Expand Up @@ -4093,6 +4098,35 @@ handoff logging completion to the BuildManager.
}
}

[Fact]
public void BuildWithZeroConnectionTimeout()
{
string contents = CleanupFileContents(@"
Forgind marked this conversation as resolved.
Show resolved Hide resolved
<Project>
<Target Name='test'>
<Message Text='Text'/>
</Target>
</Project>
");
// Do not use MSBUILDINPROCENVCHECK because this test case is expected to leave a defunct in-proc node behind.
_inProcEnvCheckTransientEnvironmentVariable.Revert();
_env.SetEnvironmentVariable("MSBUILDNODECONNECTIONTIMEOUT", "0");

BuildRequestData data = GetBuildRequestData(contents);
try
{
BuildResult result = _buildManager.Build(_parameters, data);

// The build should either finish successfully (very unlikely).
result.OverallResult.ShouldBe(BuildResultCode.Success);
}
catch (Exception e)
{
// Or it should throw InternalErrorException because the node didn't get connected within 0ms.
e.ShouldBeOfType<InternalErrorException>();
}
}

[Fact]
[ActiveIssue("https://github.com/Microsoft/msbuild/issues/4368")]
public void GraphBuildValid()
Expand Down
27 changes: 7 additions & 20 deletions src/Build/BackEnd/BuildManager/BuildManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2184,20 +2184,13 @@ private void OnThreadException(Exception e)
continue;
}

submission.CompleteLogging(false);
ladipro marked this conversation as resolved.
Show resolved Hide resolved

// Attach the exception to this submission if it does not already have an exception associated with it
if (submission.BuildResult?.Exception == null)
if (!submission.IsCompleted && submission.BuildResult != null && submission.BuildResult.Exception == null)
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't submission.BuildResult?.Exception == null mean the same thing as submission.BuildResult != null && submission.BuildResult.Exception == null? SharpLab shows an IL difference but it appears to just not be loading the BuildResult field twice in the ?. version.

Copy link
Member

Choose a reason for hiding this comment

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

If submission.BuildResult is null, submission.BuildResult?.Exception is null; ? is like <thing before> == null ? null : <thing before>.<thing after>.

(I ran this.)

Copy link
Member Author

Choose a reason for hiding this comment

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

With value type fields, one can write something like submission.BuildResult?.SomeFlag == false and this is equivalent to submission.BuildResult != null && submission.BuildResult.SomeFlag == false. But it doesn't work with reference type fields as the type of submission.BuildResult?.Exception is just Exception and if it evaluates to null, you can't tell which of the two is null.

Copy link
Member

Choose a reason for hiding this comment

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

You're right; it's the null-to-bool comparison that saves the former case which doesn't apply here.

{
if (submission.BuildResult == null)
{
submission.BuildResult = new BuildResult(submission.BuildRequest, e);
}
else
{
submission.BuildResult.Exception = _threadException.SourceException;
}
submission.BuildResult.Exception = e;
}
submission.CompleteLogging(waitForLoggingThread: false);
submission.CompleteResults(new BuildResult(submission.BuildRequest, e));
ladipro marked this conversation as resolved.
Show resolved Hide resolved

CheckSubmissionCompletenessAndRemove(submission);
}
Expand All @@ -2211,17 +2204,11 @@ private void OnThreadException(Exception e)
}

// Attach the exception to this submission if it does not already have an exception associated with it
if (submission.BuildResult?.Exception == null)
if (!submission.IsCompleted && submission.BuildResult != null && submission.BuildResult.Exception == null)
{
if (submission.BuildResult == null)
{
submission.BuildResult = new GraphBuildResult(submission.SubmissionId, e);
}
else
{
submission.BuildResult.Exception = _threadException.SourceException;
}
submission.BuildResult.Exception = e;
ladipro marked this conversation as resolved.
Show resolved Hide resolved
}
submission.CompleteResults(submission.BuildResult ?? new GraphBuildResult(submission.SubmissionId, e));

CheckSubmissionCompletenessAndRemove(submission);
}
Expand Down