Add default fallback TaskEnvironment to all IMultiThreadableTask implementations#13415
Conversation
…ementations All 19 built-in tasks implementing IMultiThreadableTask now initialize their TaskEnvironment property with a MultiProcessTaskEnvironmentDriver-backed default. This prevents NullReferenceException when tasks are: - Explicitly instantiated (e.g. new Copy()) outside the engine - Run in the out-of-proc task host via TaskHostFactory The engine's in-proc path overwrites this default in TaskExecutionHost.InitializeForBatch(), so it is purely a safety net. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds a defensive default TaskEnvironment initialization to all built-in tasks that implement IMultiThreadableTask, preventing NullReferenceException when such tasks are instantiated outside the engine or executed in the out-of-proc task host.
Changes:
- Initialize
TaskEnvironmentto aMultiProcessTaskEnvironmentDriver-backed default across 19 built-inIMultiThreadableTaskimplementations. - Add unit tests validating
TaskEnvironmentis non-null/usable for explicit instantiation and TaskHostFactory execution.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Tasks/ZipDirectory.cs | Initialize TaskEnvironment with a default fallback instance. |
| src/Tasks/XslTransformation.cs | Initialize TaskEnvironment with a default fallback instance. |
| src/Tasks/XmlPoke.cs | Initialize TaskEnvironment with a default fallback instance. |
| src/Tasks/XmlPeek.cs | Initialize TaskEnvironment with a default fallback instance. |
| src/Tasks/WriteCodeFragment.cs | Initialize TaskEnvironment with a default fallback instance. |
| src/Tasks/Unzip.cs | Initialize TaskEnvironment with a default fallback instance. |
| src/Tasks/Touch.cs | Initialize TaskEnvironment with a default fallback instance. |
| src/Tasks/RemoveDir.cs | Initialize TaskEnvironment with a default fallback instance. |
| src/Tasks/Move.cs | Initialize TaskEnvironment with a default fallback instance. |
| src/Tasks/MakeDir.cs | Initialize TaskEnvironment with a default fallback instance. |
| src/Tasks/ListOperators/FindUnderPath.cs | Initialize TaskEnvironment with a default fallback instance. |
| src/Tasks/FileIO/WriteLinesToFile.cs | Initialize TaskEnvironment with a default fallback instance. |
| src/Tasks/FileIO/VerifyFileHash.cs | Initialize TaskEnvironment with a default fallback instance. |
| src/Tasks/FileIO/ReadLinesFromFile.cs | Initialize TaskEnvironment with a default fallback instance. |
| src/Tasks/FileIO/GetFileHash.cs | Initialize TaskEnvironment with a default fallback instance. |
| src/Tasks/DownloadFile.cs | Initialize TaskEnvironment with a default fallback instance. |
| src/Tasks/Delete.cs | Initialize TaskEnvironment with a default fallback instance. |
| src/Tasks/Copy.cs | Initialize TaskEnvironment with a default fallback instance. |
| src/Tasks/AssignTargetPath.cs | Initialize TaskEnvironment with a default fallback instance. |
| src/Build.UnitTests/BackEnd/TaskHost_MultiThreadableTask_Tests.cs | Add tests validating TaskEnvironment fallback works for explicit instantiation and TaskHostFactory execution. |
You can also share your feedback on Copilot code review. Take the survey.
… tasks Update multithreaded-task-migration skill, thread-safe-tasks spec, multithreaded-msbuild spec, taskhost-threading spec, and tasks instructions to reflect that all 19 built-in IMultiThreadableTask implementations now have a default TaskEnvironment backed by MultiProcessTaskEnvironmentDriver.Instance. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: AR-May <67507805+AR-May@users.noreply.github.com>
Co-authored-by: AR-May <67507805+AR-May@users.noreply.github.com>
|
Doesn't that imply that every task that implements the interface should now do this? Means customer tasks would need to do this as well. Why can't we provide this by default when instantiating the task? |
|
Yes it implies that. We can't provide it by default because the problematic case is where customers instantiate it themselves. |
Sad panda face :( Did we ever require such a thing in the past from task authors? Is the fundamental problem here that the property on the already shipped interface has a setter and so customers can mutate it? |
we don't have other capability extending interfaces for Tasks so this is new. It modifies our ask from task authors "if you want your task to be multithreadable you should not only implement the interface but also set the default to multiproc"
No, the problem is that the internals of our tasks can't work without the environment. It's our recommended migration path is to replace cwd-based calls by our APIs, and those can't work without the environment. |
|
@rainersigwald & @baronfel I would love to hear your opinion on this. I'm not happy with this API shape honestly but sounds like there isn't a better option. |
fixes #13374
Summary
All 19 built-in tasks implementing \IMultiThreadableTask\ now initialize their \TaskEnvironment\ property with a \MultiProcessTaskEnvironmentDriver-backed default. This prevents \NullReferenceException\ when tasks are:
ew Copy()) outside the engine
The engine's in-proc path overwrites this default in \TaskExecutionHost.InitializeForBatch(), so it is purely a safety net.
Changes
Testing