Add IdleTimeout for process and functional test #277

Merged
merged 1 commit into from Jan 4, 2013

Projects

None yet

4 participants

@suwatch
Member
suwatch commented Dec 27, 2012

This is to address #151.

Kudu launches processes (thru Executable) to perform different tasks. The issue is if the process or its child never returns or hangs, it will not be terminated; worse it may hold on to some lock such as deployment blocking future operations. In Azure environment, the process will stay forever till machine reboots. This prevents the site from spinning down.

The fix is if we detect that the process is idle, we will terminate the process and its children. We define idle as there is no activity on the process output stream. The default idle timeout is 3mins and is configurable thru deployment settings manager.

90% of the change is to propagate the Idle timeout settings to executable. The main changes are in ..

Kudu.FunctionalTests/GitRepositoryManagementTests.cs
Kudu.Core/Infrastructure/Executable.cs
Kudu.Core/Infrastructure/ProcessExtensions.cs

To kill the child process, we use performance counter to figure the process tree out. This works well on PICO and Windows 7. Other alternatives are either using ManagementObject or PInvokes. However, neither work on PICO.

Unfortunately, on Windows 8, the apppool identity does not work (fail access denied when accessing perf counters). I currently makes killing child process best effort.

@amitapl amitapl commented on the diff Dec 27, 2012
Kudu.Core/Infrastructure/ProcessExtensions.cs
+ }
+ }
+
+ private static void SafeKillProcess(Process process, ITracer tracer)
+ {
+ try
+ {
+ string processName = process.ProcessName;
+ process.Kill();
+ tracer.Trace("Abort Process '{0}'.", processName);
+ }
+ catch (Exception)
+ {
+ if (!process.HasExited)
+ {
+ throw;
@amitapl
amitapl Dec 27, 2012 Member

Why not trace and swallow the exception here?

@suwatch
suwatch Dec 28, 2012 Member

The contract is kill process will gaurantee either process terminated or throw (an issue that our team needs to investigate). There is a race where the process may terminate naturally and we call kill on it. In that case, we handle with HasExited.

@amitapl
amitapl Dec 28, 2012 Member

So why is it called Safe? I thought that means it won't throw an exception?

@suwatch
suwatch Jan 1, 2013 Member

Safe means it does not throw as long as process did die. Perhaps Safe is too subjective to be used.

@davidfowl davidfowl and 1 other commented on an outdated diff Dec 28, 2012
Kudu.Core/Infrastructure/ProcessExtensions.cs
+ int parentId = FindPidFromIndexedProcessName(indexedProcessName);
+ List<int> children = null;
+ if (!tree.TryGetValue(parentId, out children))
+ {
+ tree[parentId] = children = new List<int>();
+ }
+
+ children.Add(proc.Key);
+ }
+
+ return tree;
+ }
+
+ private static string FindIndexedProcessName(int pid, string processName)
+ {
+ string processIndexdName = null;
@davidfowl davidfowl and 1 other commented on an outdated diff Dec 28, 2012
Kudu.Core/Infrastructure/ProcessExtensions.cs
+ string processName = process.ProcessName;
+ process.Kill();
+ tracer.Trace("Abort Process '{0}'.", processName);
+ }
+ catch (Exception)
+ {
+ if (!process.HasExited)
+ {
+ throw;
+ }
+ }
+ }
+
+ private static Dictionary<int, List<int>> GetProcessTree()
+ {
+ Dictionary<int, List<int>> tree = new Dictionary<int, List<int>>();
@davidfowl davidfowl and 1 other commented on an outdated diff Dec 28, 2012
Kudu.Core/Infrastructure/Executable.cs
+
+#if !SITEMANAGEMENT
+ class IdleManager
+ {
+ private static int WaitInterval = 5000;
+ private readonly string _processName;
+ private readonly TimeSpan _idleTimeout;
+ private readonly ITracer _tracer;
+ private DateTime _lastActivity;
+
+ public IdleManager(string path, TimeSpan idleTimeout, ITracer tracer)
+ {
+ _processName = new FileInfo(path).Name;
+ _idleTimeout = idleTimeout;
+ _tracer = tracer;
+ _lastActivity = DateTime.Now;
@davidfowl
davidfowl Dec 28, 2012 Member

Doesn't everything else use UtcNow (it's much faster, although it really wouldn't affect performance here)?

@suwatch
suwatch Jan 1, 2013 Member

UtcNow it is.

@davidfowl davidfowl commented on an outdated diff Dec 28, 2012
Kudu.Core/Infrastructure/Executable.cs
@@ -63,14 +65,29 @@ public void SetHomePath(string homePath)
var process = CreateProcess(arguments, args);
process.Start();
- Func<StreamReader, string> reader = (StreamReader streamReader) => streamReader.ReadToEnd();
+#if !SITEMANAGEMENT
+ var idleManager = new IdleManager(Path, IdleTimeout, tracer);
+#else
+ var idleManager = new IdleManager();
+#endif
+ Func<StreamReader, string> reader = (StreamReader streamReader) =>
+ {
+ var strb = new StringBuilder();
+ while (!streamReader.EndOfStream)
+ {
+ idleManager.UpdateActivity();
+ strb.Append((char)streamReader.Read());
@davidfowl
davidfowl Dec 28, 2012 Member

You read a char at a time?

@pranavkm pranavkm commented on the diff Jan 2, 2013
Kudu.Contracts/Settings/DeploymentSettingsExtension.cs
@@ -6,6 +6,7 @@ namespace Kudu.Contracts.Settings
{
public static class DeploymentSettingsExtension
{
+ public static TimeSpan DefaultCommandIdleTimeout = TimeSpan.FromSeconds(180);
@pranavkm
pranavkm Jan 2, 2013 Contributor

private readonly?

@suwatch
suwatch Jan 2, 2013 Member

Consistent with the rest. besides, test is using this default value.

@pranavkm pranavkm commented on the diff Jan 2, 2013
Kudu.Core/Infrastructure/ProcessExtensions.cs
+ try
+ {
+ string processName = process.ProcessName;
+ process.Kill();
+ tracer.Trace("Abort Process '{0}'.", processName);
+ }
+ catch (Exception)
+ {
+ if (!process.HasExited)
+ {
+ throw;
+ }
+ }
+ }
+
+ private static Dictionary<int, List<int>> GetProcessTree()
@pranavkm
pranavkm Jan 2, 2013 Contributor

SO has a bit about using WMI to look up child processes http://stackoverflow.com/a/5921994. It does look quite a bit simpler than this.

@suwatch
suwatch Jan 2, 2013 Member

does not work on PICO (see description above)

@suwatch suwatch merged commit 8e2f22f into master Jan 4, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment