[Bug]: Please re-initilize "_terminatedTool" in class Microsoft.Build.Utilities.ToolTask

[gpwen]

Issue Description

In the following code:
https://github.com/dotnet/msbuild/blob/main/src/Utilities/ToolTask.cs

The private field _terminatedTool has not be initialized in method ExecuteTool(). As a result, in rare cases where:

1. ToolTask.Execute() being called multiple times from derived classes, and;
2. The tool execution being timed out once.

Such timed-out state will be incorrectly permanent, impacting all following ToolTask.Execute() calls, and forcing the method to return false even if the execution succeeded.

The problem can be easily addressed by reinitializing _terminatedTool field to false in method ExecuteTool(), along with other private fields.

Thanks!

Steps to Reproduce

1. Create a CLI tool that would wait the amount of time (ms) specified on command line;
2. Create a ToolTask derived class to execute that tool;
3. Override Execute(), calling base.Execute() multiple times to execute the tool multiple times;
4. In the first execution, set delay to a larger value (>100s), and set it to smaller value in all subsequent execution (< 1s);
5. Set Timeout of the task to 5s, so the first call will timed out, and subsequent ones won't;
6. Watch how all subsequent base.Execute() returns false, so the task never succeeds.

Expected Behavior

The first execution timed out (and fail), but subsequent execution should succeed.

Actual Behavior

After the first execution timed out, all subsequent execution failed even if they actually succeeded.

Analysis

Private field _terminatedTool has not been reinitialized correctly in ToolTask.ExecuteTool() method.

Versions & Configurations

The latest version (17.5).

[gpwen]

The problem found in MSBuild version 17.4.1.60106, but it's the same in the latest version.

[rainersigwald]

I think that would be a reasonable change, though ToolTask is not particularly oriented to running multiple tools. @gpwen would you be interested in submitting a PR?

[gpwen]

Thanks for considering the change. I'll submit a PR.

Some background: I need to execute an external command repeatedly (predefined number of maximum repeats) with timeout until it succeeds. Not sure if I can execute another MSBuild task from within a task, which would look better. I called base.Execute() multiple times as a workaround. I know it's probably not a good idea after read through the code. Really appreciate if you can suggest an alternative.

[gpwen]

I don't know there are Linux/Mac tests. Sorry for the unit test break in the first commit. I'm more than happy to make those work. The second commit should do the trick.

[rainersigwald]

Not sure if I can execute another MSBuild task from within a task

It is possible to do this, by manually calling the constructor and property accessors of the task, including setting BuildEngine

msbuild/src/Framework/ITask.cs

Lines 14 to 18 in 1a6d753

	/// <summary>
	/// This property is set by the build engine to allow a task to call back into it.
	/// </summary>
	/// <value>The interface on the build engine available to tasks.</value>
	IBuildEngine BuildEngine

and then calling Execute(). However, I don't generally recommend it and it doesn't seem obviously better than your current retrying-ToolTask approach.

[gpwen]

Thanks for the information! It's good to know I can do that as well.