From aab0d9a6b7eb36bd659a6371a14e0091f9134dc8 Mon Sep 17 00:00:00 2001 From: Guopeng Wen Date: Wed, 8 Mar 2023 12:30:24 +0800 Subject: [PATCH 1/2] Reset internal terminated flag in ToolTask (#8541) 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 https://github.com/dotnet/msbuild/issues/8541 --- src/Utilities.UnitTests/ToolTask_Tests.cs | 128 +++++++++++++++++++++- src/Utilities/ToolTask.cs | 1 + 2 files changed, 128 insertions(+), 1 deletion(-) diff --git a/src/Utilities.UnitTests/ToolTask_Tests.cs b/src/Utilities.UnitTests/ToolTask_Tests.cs index f1d08123d54..736381acc97 100644 --- a/src/Utilities.UnitTests/ToolTask_Tests.cs +++ b/src/Utilities.UnitTests/ToolTask_Tests.cs @@ -117,7 +117,7 @@ protected override int ExecuteTool(string pathToTool, string responseFileCommand StartInfo = GetProcessStartInfo(GenerateFullPathToTool(), NativeMethodsShared.IsWindows ? "/x" : string.Empty, null); return result; } - }; + } [Fact] public void Regress_Mutation_UserSuppliedToolPathIsLogged() @@ -825,5 +825,131 @@ protected override string GenerateCommandLineCommands() return $"echo łoł > {OutputPath}"; } } + + /// + /// Verifies that a ToolTask instance can return correct results when executed multiple times with timeout. + /// + /// Specifies the number of repeats for external command execution. + /// Delay to generate on the first execution in milliseconds. + /// Delay to generate on follow-up execution in milliseconds. + /// Task timeout in milliseconds. + /// + /// These tests execute the same task instance multiple times, which will in turn run a PowerShell command to + /// sleep predefined amount of time. The first execution may time out, but all following ones won't. It is + /// expected that all following executions return success. + /// + [Theory] + [InlineData(1, 1, 1, -1)] // Normal case, no repeat. + [InlineData(3, 1, 1, -1)] // Repeat without timeout. + [InlineData(3, 10000, 1, 1000)] // Repeat with timeout. + public void ToolTaskThatTimeoutAndRetry(int repeats, int initialDelay, int followupDelay, int timeout) + { + using var env = TestEnvironment.Create(_output); + + // Task under test: + var task = new ToolTaskThatRetry + { + BuildEngine = new MockEngine(), + InitialDelay = initialDelay, + FollowupDelay = followupDelay, + Timeout = timeout + }; + + // Execute the same task instance multiple times. The index is one-based. + bool result; + for (int i = 1; i <= repeats; i++) + { + // Execute the task: + result = task.Execute(); + task.RepeatCount.ShouldBe(i); + + // The first execution may fail (timeout), but all following ones should succeed: + if (i > 1) + { + result.ShouldBeTrue(); + task.ExitCode.ShouldBe(0); + } + } + } + + /// + /// A simple implementation of to run PowerShell to generate programmable delay. + /// + /// + /// This task to run PowerShell "Start-Sleep" command to delay for predefined, variable amount of time based on + /// how many times the instance has been executed. + /// + private sealed class ToolTaskThatRetry : ToolTask + { + // Test with PowerShell to generate a programmable delay: + private readonly string _powerShell = "PowerShell.exe"; + + // PowerShell command to sleep: + private readonly string _sleepCommand = "-ExecutionPolicy RemoteSigned -Command \"Start-Sleep -Milliseconds {0}\""; + + public ToolTaskThatRetry() + : base() + { + } + + /// + /// Gets or sets the delay for the first execution. + /// + /// + /// Defaults to 10 seconds. + /// + public Int32 InitialDelay { get; set; } = 10000; + + /// + /// Gets or sets the delay for the follow-up executions. + /// + /// + /// Defaults to 1 milliseconds. + /// + public Int32 FollowupDelay { get; set; } = 1; + + /// + /// Int32 output parameter for the execution repeat counter for test purpose. + /// + [Output] + public Int32 RepeatCount { get; private set; } = 0; + + /// + /// Gets the tool name (PowerShell). + /// + protected override string ToolName => _powerShell; + + /// + /// Search path for PowerShell. + /// + /// + /// This only works on Windows. + /// + protected override string GenerateFullPathToTool() => FindOnPath(_powerShell); + + /// + /// Generate a PowerShell command to sleep different amount of time based on execution counter. + /// + protected override string GenerateCommandLineCommands() => + string.Format(_sleepCommand, RepeatCount < 2 ? InitialDelay : FollowupDelay); + + /// + /// Ensures that test parameters make sense. + /// + protected internal override bool ValidateParameters() => + (InitialDelay > 0) && (FollowupDelay > 0) && base.ValidateParameters(); + + /// + /// Runs external command (PowerShell) to generate programmable delay. + /// + /// + /// true if the task runs successfully; false otherwise. + /// + public override bool Execute() + { + RepeatCount++; + return base.Execute(); + } + } } } diff --git a/src/Utilities/ToolTask.cs b/src/Utilities/ToolTask.cs index 2243faedfca..0abb6213e67 100644 --- a/src/Utilities/ToolTask.cs +++ b/src/Utilities/ToolTask.cs @@ -670,6 +670,7 @@ protected virtual int ExecuteTool( _standardOutputDataAvailable = new ManualResetEvent(false); _toolExited = new ManualResetEvent(false); + _terminatedTool = false; _toolTimeoutExpired = new ManualResetEvent(false); _eventsDisposed = false; From 54538b57f27795f6219a96971dcb10549cb2b9bd Mon Sep 17 00:00:00 2001 From: Guopeng Wen Date: Wed, 8 Mar 2023 14:10:09 +0800 Subject: [PATCH 2/2] Fix unit tests for #8541 on UNIX-like systems --- src/Utilities.UnitTests/ToolTask_Tests.cs | 52 ++++++++++++----------- 1 file changed, 28 insertions(+), 24 deletions(-) diff --git a/src/Utilities.UnitTests/ToolTask_Tests.cs b/src/Utilities.UnitTests/ToolTask_Tests.cs index 736381acc97..06811207770 100644 --- a/src/Utilities.UnitTests/ToolTask_Tests.cs +++ b/src/Utilities.UnitTests/ToolTask_Tests.cs @@ -834,9 +834,9 @@ protected override string GenerateCommandLineCommands() /// Delay to generate on follow-up execution in milliseconds. /// Task timeout in milliseconds. /// - /// These tests execute the same task instance multiple times, which will in turn run a PowerShell command to - /// sleep predefined amount of time. The first execution may time out, but all following ones won't. It is - /// expected that all following executions return success. + /// These tests execute the same task instance multiple times, which will in turn run a shell command to sleep + /// predefined amount of time. The first execution may time out, but all following ones won't. It is expected + /// that all following executions return success. /// [Theory] [InlineData(1, 1, 1, -1)] // Normal case, no repeat. @@ -847,7 +847,7 @@ public void ToolTaskThatTimeoutAndRetry(int repeats, int initialDelay, int follo using var env = TestEnvironment.Create(_output); // Task under test: - var task = new ToolTaskThatRetry + var task = new ToolTaskThatSleeps { BuildEngine = new MockEngine(), InitialDelay = initialDelay, @@ -873,23 +873,28 @@ public void ToolTaskThatTimeoutAndRetry(int repeats, int initialDelay, int follo } /// - /// A simple implementation of to run PowerShell to generate programmable delay. + /// A simple implementation of to sleep for a while. /// /// - /// This task to run PowerShell "Start-Sleep" command to delay for predefined, variable amount of time based on - /// how many times the instance has been executed. + /// This task runs shell command to sleep for predefined, variable amount of time based on how many times the + /// instance has been executed. /// - private sealed class ToolTaskThatRetry : ToolTask + private sealed class ToolTaskThatSleeps : ToolTask { - // Test with PowerShell to generate a programmable delay: - private readonly string _powerShell = "PowerShell.exe"; - // PowerShell command to sleep: - private readonly string _sleepCommand = "-ExecutionPolicy RemoteSigned -Command \"Start-Sleep -Milliseconds {0}\""; + private readonly string _powerShellSleep = "-ExecutionPolicy RemoteSigned -Command \"Start-Sleep -Milliseconds {0}\""; + + // UNIX command to sleep: + private readonly string _unixSleep = "-c \"sleep {0}\""; + + // Full path to shell: + private readonly string _pathToShell; - public ToolTaskThatRetry() + public ToolTaskThatSleeps() : base() { + // Determines shell to use: PowerShell for Windows, sh for UNIX-like systems: + _pathToShell = NativeMethodsShared.IsUnixLike ? "/bin/sh" : FindOnPath("PowerShell.exe"); } /// @@ -909,29 +914,28 @@ public ToolTaskThatRetry() public Int32 FollowupDelay { get; set; } = 1; /// - /// Int32 output parameter for the execution repeat counter for test purpose. + /// Int32 output parameter for the repeat counter for test purpose. /// [Output] public Int32 RepeatCount { get; private set; } = 0; /// - /// Gets the tool name (PowerShell). + /// Gets the tool name (shell). /// - protected override string ToolName => _powerShell; + protected override string ToolName => Path.GetFileName(_pathToShell); /// - /// Search path for PowerShell. + /// Gets the full path to shell. /// - /// - /// This only works on Windows. - /// - protected override string GenerateFullPathToTool() => FindOnPath(_powerShell); + protected override string GenerateFullPathToTool() => _pathToShell; /// - /// Generate a PowerShell command to sleep different amount of time based on execution counter. + /// Generates a shell command to sleep different amount of time based on repeat counter. /// protected override string GenerateCommandLineCommands() => - string.Format(_sleepCommand, RepeatCount < 2 ? InitialDelay : FollowupDelay); + NativeMethodsShared.IsUnixLike ? + string.Format(_unixSleep, RepeatCount < 2 ? InitialDelay / 1000.0 : FollowupDelay / 1000.0) : + string.Format(_powerShellSleep, RepeatCount < 2 ? InitialDelay : FollowupDelay); /// /// Ensures that test parameters make sense. @@ -940,7 +944,7 @@ protected internal override bool ValidateParameters() => (InitialDelay > 0) && (FollowupDelay > 0) && base.ValidateParameters(); /// - /// Runs external command (PowerShell) to generate programmable delay. + /// Runs shell command to sleep for a while. /// /// /// true if the task runs successfully; false otherwise.