Don't hang builds on BuildManager internal errors #5917
Conversation
| // 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
If submission.BuildResult is null, submission.BuildResult?.Exception is null; ? is like <thing before> == null ? null : <thing before>.<thing after>.
(I ran this.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
You're right; it's the null-to-bool comparison that saves the former case which doesn't apply here.
Co-authored-by: Rainer Sigwald <raines@microsoft.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
The logic to handle exceptions thrown on logging and communication threads was trying to make the build submission fail with the exception but it didn't always complete it. This manifested as the build hanging, for example when building with
MSBUILDNODECONNECTIONTIMEOUT = 0, which is almost guaranteed to throwInternalErrorExceptioninInstantiateNode.This PR fixes it by making sure that all submissions are completed in
OnThreadException. Unlike the original code, proper care is taken to avoid race conditions - i.e. the build result is mutated only if hasn't already been passed to the completion handler.Fixes #5911