Attribute custom task OOM (and other critical exceptions) to the task #9965
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #9928
Context
OOM in custom tasks is rethrown to RequestBuilder, where it is logged as an MSBuild bug.
We might try - on best effort basis - to log an error for the task little more up the stack and then continue in unrolling as before. If we succeed - we get better logging experience for the case; if we fail - we get the same experience as before.
Changes Made
TaskBuilder
RequestThreadProc
andBuildAndReport
methods merged into single one - so that writing the 'error dump' file (MSBuild***.failure.txt) is deterministically written before reporting the result backDebugUtils.DebugPath
- as this was set before any unit test was given chance to start, so injecting altering env var was not possible.Testing
Added test with dummy OOM (not a real CLR OOM - in order to have deterministic test outputs)
Notes
Discussed the OOM handling with @janvorli (for a perspective from the Runtime PoV) - we should not make no expectations that anything can succeed after OOM catching (though this migh get more successful in single threaded processing - which MSBuild currently is), but if we are fine with the handler possibly failing (which basically leads to pre-existing behavior) - than there should not be any security nor corectness concerns.