Skip to content

Commit

Permalink
Convert script action timeout to TimeSpan early in the script executi…
Browse files Browse the repository at this point in the history 
…on workflow.

Add *Nix test to CommandLineRunnerFixture that is functional equivalent of Windows timeout test.
  • Loading branch information
@miguelelvir
miguelelvir committed Feb 2, 2020
1 parent 52d111a commit 500e975
Show file tree
Hide file tree
Showing 7 changed files with 78 additions and 57 deletions.
2 changes: 1 addition & 1 deletion source/Calamari.Shared/Integration/Processes/CommandLineException.cs
View file
Expand Up @@ -33,7 +33,7 @@ public class CommandLineException : Exception

if (timedOut)
{
sb.Append("Timed out before execution completed. Check the script Timeout environment variable.").AppendLine();
sb.Append("Timed out before execution completed. Check the Octopus.Action.Script.Timeout variable.").AppendLine();
}

sb.Append("Failed with exit code: ").Append(exitCode).AppendLine();
Expand Down
11 changes: 5 additions & 6 deletions source/Calamari.Shared/Integration/Processes/CommandLineInvocation.cs
View file
@@ -1,21 +1,20 @@
using System;
using System.Collections.Generic;
using System.Security;
using System.Threading;

namespace Calamari.Integration.Processes
{
public class CommandLineInvocation
{
readonly string workingDirectory;

public CommandLineInvocation(string executable, string arguments, Dictionary<string, string> environmentVars = null, bool isolate = false, int timeoutMilliseconds = Timeout.Infinite)
public CommandLineInvocation(string executable, string arguments, Dictionary<string, string> environmentVars = null, bool isolate = false, TimeSpan? timeout = null)
{
Executable = executable;
Arguments = arguments;
EnvironmentVars = environmentVars;
Isolate = isolate;
TimeoutMilliseconds = timeoutMilliseconds;
Timeout = timeout ?? TimeSpan.MaxValue;
}

public CommandLineInvocation(
Expand All @@ -26,13 +25,13 @@ public CommandLineInvocation(string executable, string arguments, Dictionary<str
string userName = null,
SecureString password = null,
bool isolate = false,
int timeoutMilliseconds = Timeout.Infinite)
TimeSpan? timeout = null)
: this(executable, arguments, environmentVars, isolate)
{
this.workingDirectory = workingDirectory;
UserName = userName;
Password = password;
TimeoutMilliseconds = timeoutMilliseconds;
Timeout = timeout ?? TimeSpan.MaxValue;
}

public string Executable { get; }
Expand All @@ -45,7 +44,7 @@ public CommandLineInvocation(string executable, string arguments, Dictionary<str

public Dictionary<string, string> EnvironmentVars { get; }
public bool Isolate { get; }
public int TimeoutMilliseconds { get; internal set; }
public TimeSpan Timeout { get; internal set; }

/// <summary>
/// The initial working-directory for the invocation.
Expand Down
22 changes: 4 additions & 18 deletions source/Calamari.Shared/Integration/Processes/CommandLineRunner.cs
View file
Expand Up @@ -14,19 +14,6 @@ public CommandLineRunner(ICommandOutput commandOutput)

public CommandResult Execute(CommandLineInvocation invocation)
{
var timedOut = false;

if (invocation.TimeoutMilliseconds > -1)
{
if (invocation.TimeoutMilliseconds == 0)
{
var link = "https://docs.microsoft.com/en-us/dotnet/api/system.diagnostics.process.waitforexit?view=netframework-4.8#System_Diagnostics_Process_WaitForExit_System_Int32_";
commandOutput.WriteInfo($"The timeout for this script was set to 0. Perhaps this was not intended. Setting the timeout to 0 will succeed only if the script exits immediately. See {link}");
}

commandOutput.WriteInfo($"The script for this action will be executed with a timeout of {invocation.TimeoutMilliseconds} milliseconds. To remove this timeout, set the Action.Script.Timeout special variable to -1 or delete the variable.");
}

try
{
var exitCode = SilentProcessRunner.ExecuteCommand(
Expand All @@ -38,15 +25,14 @@ public CommandResult Execute(CommandLineInvocation invocation)
invocation.Password,
commandOutput.WriteInfo,
commandOutput.WriteError,
invocation.TimeoutMilliseconds);

timedOut = exitCode.TimedOut;
invocation.Timeout);

return new CommandResult(
invocation.ToString(),
exitCode.ExitCode,
exitCode.ErrorOutput,
invocation.WorkingDirectory);
invocation.WorkingDirectory,
exitCode.TimedOut);
}
catch (Exception ex)
{
Expand All @@ -63,7 +49,7 @@ public CommandResult Execute(CommandLineInvocation invocation)
-1,
ex.ToString(),
invocation.WorkingDirectory,
timedOut);
false);
}
}

Expand Down
36 changes: 15 additions & 21 deletions source/Calamari.Shared/Integration/Processes/SilentProcessRunner.cs
View file
Expand Up @@ -43,9 +43,9 @@ static SilentProcessRunner()
string workingDirectory,
Action<string> output,
Action<string> error,
int timeoutMilliseconds = Timeout.Infinite)
TimeSpan? timeout = null)
{
return ExecuteCommand(executable, arguments, workingDirectory, null, null, null, output, error, timeoutMilliseconds);
return ExecuteCommand(executable, arguments, workingDirectory, null, null, null, output, error, timeout);
}

public static SilentProcessRunnerResult ExecuteCommand(
Expand All @@ -55,9 +55,9 @@ static SilentProcessRunner()
Dictionary<string, string> environmentVars,
Action<string> output,
Action<string> error,
int timeoutMilliseconds = Timeout.Infinite)
TimeSpan? timeout = null)
{
return ExecuteCommand(executable, arguments, workingDirectory, environmentVars, null, null, output, error, timeoutMilliseconds);
return ExecuteCommand(executable, arguments, workingDirectory, environmentVars, null, null, output, error, timeout);
}

public static SilentProcessRunnerResult ExecuteCommand(
Expand All @@ -69,7 +69,7 @@ static SilentProcessRunner()
SecureString password,
Action<string> output,
Action<string> error,
int timeoutMilliseconds = Timeout.Infinite)
TimeSpan? timeout = null)
{
try
{
Expand Down Expand Up @@ -152,27 +152,21 @@ static SilentProcessRunner()
process.BeginOutputReadLine();
process.BeginErrorReadLine();

// Some processes can have race conditions. Between the call to Start and WaitForExit part of the process may have exited already.
// This can happen when callign CommitChanges in dotnet's ServerManager as shown here: https://stackoverflow.com/questions/7446632/servermanager-commitchanges-makes-changes-with-a-slight-delay
// Commit changes is called by appcmd from IIS, which is called by Set-ItemProperty from the WebAdministration module in Powershell
// https://docs.microsoft.com/en-us/dotnet/api/system.diagnostics.process.waitforexit?view=netframework-4.8#System_Diagnostics_Process_WaitForExit_System_Int32_
// Add a timeout passed down from some default configuration or context up the call stack, 0 will never timeout.
var timedOut = !process.WaitForExit(timeoutMilliseconds);
// TimeSpan.TotalMilliseconds can have a value > int.MaxValue, so we just assume wait for ever if this happens.
var timeoutMilliseconds = timeout == null || timeout.Value.TotalMilliseconds > int.MaxValue ? -1 : (int)(timeout.Value.TotalMilliseconds);
var processExited = process.WaitForExit(timeoutMilliseconds);

if (!timedOut)
if (!processExited)
{
// Only wait when the process has not timed out, otherwise we will be stuck here as well.
outputWaitHandle.WaitOne();
errorWaitHandle.WaitOne();
} else
{
Log.Error($"Process with ID {process.Id} exceeded the max allowed runtime of {timeoutMilliseconds} milliseconds and will be killed.");
process.CancelOutputRead();
process.CancelErrorRead();
Log.Error($"Process with ID {process.Id} exceeded the max allowed runtime of {timeout} and will be killed.");
process.Kill();
process.WaitForExit();
}

return new SilentProcessRunnerResult(process.ExitCode, errorData.ToString(), timedOut);
outputWaitHandle.WaitOne();
errorWaitHandle.WaitOne();

return new SilentProcessRunnerResult(process.ExitCode, errorData.ToString(), !processExited);
}
}
}
Expand Down
20 changes: 12 additions & 8 deletions source/Calamari.Shared/Integration/Scripting/ScriptEngine.cs
View file
@@ -1,7 +1,6 @@
using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Threading;
using Calamari.Deployment;
using Calamari.Integration.FileSystem;
using Calamari.Integration.Processes;
Expand Down Expand Up @@ -35,12 +34,17 @@ public abstract class ScriptEngine : IScriptEngine
if (variables.IsSet(SpecialVariables.Action.Script.Timeout))
{
var timeout = variables.GetInt32(SpecialVariables.Action.Script.Timeout);
execution.CommandLineInvocation.TimeoutMilliseconds = timeout ?? Timeout.Infinite;
Log.Info($"Timeout was set to {execution.CommandLineInvocation.TimeoutMilliseconds}");
}
else
{
Log.Verbose("Timeout was not set for this script. Octopus will wait for the script to complete indefinitely.");
execution.CommandLineInvocation.Timeout = timeout.HasValue && timeout > 0 ? TimeSpan.FromMilliseconds(timeout.Value) : TimeSpan.MaxValue;

if (execution.CommandLineInvocation.Timeout == TimeSpan.Zero)
{
var link = "https://docs.microsoft.com/en-us/dotnet/api/system.diagnostics.process.waitforexit?view=netframework-4.8#System_Diagnostics_Process_WaitForExit_System_Int32_";
Log.Warn($"The timeout for this script was set to 0. Perhaps this was not intended. Setting the timeout to 0 will succeed only if the script exits immediately. See {link}. It will be ignored.");
}
else if (execution.CommandLineInvocation.Timeout > TimeSpan.Zero && execution.CommandLineInvocation.Timeout != TimeSpan.MaxValue)
{
Log.Verbose($"The script for this action will be executed with a timeout of {execution.CommandLineInvocation.Timeout} milliseconds. To remove this timeout, set the Action.Script.Timeout special variable to -1 or delete the variable.");
}
}

try
Expand Down
1 change: 1 addition & 0 deletions source/Calamari.Tests/Calamari.Tests.csproj
View file
Expand Up @@ -33,6 +33,7 @@
<PackageReference Include="Assent" Version="1.3.1" />
<PackageReference Include="Polly" Version="5.4.0" />
<PackageReference Include="System.ComponentModel.TypeConverter" Version="4.3.0" />
<PackageReference Include="System.Runtime.InteropServices.RuntimeInformation" Version="4.3.0" />
</ItemGroup>
<ItemGroup Condition=" '$(TargetFrameworkIdentifier)' == '.NETCoreApp' ">
<PackageReference Include="Markdown" Version="2.1.0" />
Expand Down
43 changes: 40 additions & 3 deletions source/Calamari.Tests/Fixtures/Integration/Process/CommandLineRunnerFixture.cs
View file
Expand Up @@ -3,7 +3,9 @@
using Calamari.Tests.Helpers;
using FluentAssertions;
using NUnit.Framework;
using System;
using System.Collections.Generic;
using System.Runtime.InteropServices;

namespace Calamari.Tests.Fixtures.Integration.Process
{
Expand All @@ -22,19 +24,54 @@ public void ScriptShouldFailIfExecutableDoesNotExist()
}

[Test]
public void ScriptShouldFailWhenTimeoutIsSpecifiedAfterSomeTime()
[Category(TestCategory.CompatibleOS.OnlyWindows)]
public void ScriptShouldFailWhenTimeoutIsSpecifiedAfterSomeTimeInWindowsSystems()
{
// For the sake of those running it with test adapters
if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX) || RuntimeInformation.IsOSPlatform(OSPlatform.Linux))
{
return;
}

// Windows function. See TIMEOUT /?
const string executable = "TIMEOUT";
var output = new CaptureCommandOutput();
var subject = new CommandLineRunner(output);

// Set a timeout of 500 milliseconds
var environmentVars = new Dictionary<string, string> { { SpecialVariables.Action.Script.Timeout, "500" } };
var environmentVars = new Dictionary<string, string> { };
var timeout = TimeSpan.FromMilliseconds(500);

// Run a function with a 100 second timeout, which should exit before it completes due to the configured timeout.
// This simulates stuck deployments on a larger scale (say stuck for 24 hours).
var result = subject.Execute(new CommandLineInvocation(executable: executable, arguments: "100 /NOBREAK", environmentVars: environmentVars, timeout: timeout));
result.HasErrors.Should().BeFalse();
result.TimedOut.Should().BeTrue();
output.Errors.Should().BeEmpty();
}

[Test]
[Category(TestCategory.CompatibleOS.OnlyNixOrMac)]
public void ScriptShouldFailWhenTimeoutIsSpecifiedAfterSomeTimeInNixSystems()
{
// For the sake of those running it with test adapters
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
return;
}

// *Nix function. See `man sleep`
const string executable = "sleep";
var output = new CaptureCommandOutput();
var subject = new CommandLineRunner(output);

// Set a timeout of 500 milliseconds
var environmentVars = new Dictionary<string, string> { };
var timeout = TimeSpan.FromMilliseconds(500);

// Run a function with a 100 second timeout, which should exit before it completes due to the configured timeout.
// This simulates stuck deployments on a larger scale (say stuck for 24 hours).
var result = subject.Execute(new CommandLineInvocation(executable: executable, arguments: "100 /NOBREAK", environmentVars: environmentVars));
var result = subject.Execute(new CommandLineInvocation(executable: executable, arguments: "100", environmentVars: environmentVars, timeout: timeout));
result.HasErrors.Should().BeFalse();
result.TimedOut.Should().BeTrue();
output.Errors.Should().BeEmpty();
Expand Down

0 comments on commit 500e975

Please sign in to comment.