Reset internal terminated flag in ToolTask (#8541) #8544
Conversation
ToolTask uses a private flag "_terminatedTool" to indicate the task execution timed out or cancelled. That flag should be reset on each execution, otherwise the return value of all following executions could be changed. Fixes dotnet#8541
|
@dotnet-policy-service agree |
| result = task.Execute(); | ||
| task.RepeatCount.ShouldBe(i); | ||
|
|
||
| // The first execution may fail (timeout), but all following ones should succeed: |
There was a problem hiding this comment.
In tests the ideal timeouts are infinitely large so they never hit or infinitely small (1ms) so they always hit or will only hit if the test discovers a bug. Is that possible?
There was a problem hiding this comment.
Per #8546, the timeout isn't properly used at the moment. We could consider delaying this PR until the other one is in?
There was a problem hiding this comment.
In tests the ideal timeouts are infinitely large so they never hit or infinitely small (1ms) so they always hit or will only hit if the test discovers a bug. Is that possible?
Hi @danmoseley, the test case will run sleep command to create some delay. It's possible to let the command sleep infinitely, it's controlled by XUnit inline parameters. However, if Timeout failed when infinite delay is in use, the test will hang forever. So I set that to 10s, with a timeout of 1s, just to be safe. Yes, there's certain possibility that the timeout won't play out as planned. Just some tradeoff.
Do you think I should set that delay in case 3 to a larger number (like 1 hour)?
| result = task.Execute(); | ||
| task.RepeatCount.ShouldBe(i); | ||
|
|
||
| // The first execution may fail (timeout), but all following ones should succeed: |
There was a problem hiding this comment.
Per #8546, the timeout isn't properly used at the moment. We could consider delaying this PR until the other one is in?
| @@ -670,6 +670,7 @@ private string GetTemporaryResponseFile(string responseFileCommands, out string | |||
| _standardOutputDataAvailable = new ManualResetEvent(false); | |||
|
|
|||
| _toolExited = new ManualResetEvent(false); | |||
| _terminatedTool = false; | |||
There was a problem hiding this comment.
Are there any other things that need to be reset that currently aren't being reset? I'm wondering if we should move this to a separate "ResetToolTaskState" method.
There was a problem hiding this comment.
I've checked all related working variables (fields), I think all of them should be handled for now.
Forgind
added
the
merge-when-branch-open
ToolTask uses a private flag
_terminatedToolto indicate the task execution timed out or cancelled. That flag should be reset on each execution, otherwise the return value of all following executions could be changed.Fixes #8541
Context
When the same ToolTask instance being executed multiple times (by derived classes, for example), the return status of all following executions might be wrong (
false) if the first execution timed-out or cancelled. Such case may arise if user try to run an external tool with retry and timeout. The problem is the internal_terminatedToolflag has not been reset on each execution. Reset that flag on each execution solved the problem.Changes Made
Resets the internal flag
ToolTask._terminatedToolon each task execution.Testing
The following unit test has been added:
Microsoft.Build.UnitTests.ToolTaskThatTimeoutAndRetry. It has 3 cases that verify no repeated execution, repeated execution with or without timeout. It's the last case that has been fixed, the rest is for regression.All ToolTask unit tests passed.
Notes
Timeoutsetting in the unit test might be tricky. On slow hardware you may want to set that to a larger value;