Skip to content

Attribute custom task OOM (and other critical exceptions) to the task - #9965

Merged
JanKrivanek merged 8 commits into
dotnet:mainfrom
JanKrivanek:proto/handle-oom
Apr 5, 2024
Merged

JanKrivanek merged 8 commits into
dotnet:mainfrom
JanKrivanek:proto/handle-oom

Conversation

@JanKrivanek

@JanKrivanek JanKrivanek commented Apr 2, 2024 •

Copy link
Copy Markdown
Member

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

  • The exception is attempted to be logged in TaskBuilder
  • RequestThreadProc and BuildAndReport methods merged into single one - so that writing the 'error dump' file (MSBuild***.failure.txt) is deterministically written before reporting the result back
  • Exposing an internal option to re-initialize DebugUtils.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.

@JanKrivanek
JanKrivanek marked this pull request as ready for review April 3, 2024 09:28
Comment thread src/Framework/CriticalTaskException.cs
Comment thread src/Build.UnitTests/BackEnd/TaskExecutionHost_Tests.cs
Comment thread src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs
@JanKrivanek
JanKrivanek merged commit 494047f into dotnet:main Apr 5, 2024
@JanKrivanek
JanKrivanek deleted the proto/handle-oom branch April 5, 2024 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants