diff --git a/src/DynamoUtilities/CLIWrapper.cs b/src/DynamoUtilities/CLIWrapper.cs
new file mode 100644
index 00000000000..5578e233867
--- /dev/null
+++ b/src/DynamoUtilities/CLIWrapper.cs
@@ -0,0 +1,175 @@
+using System;
+using System.ComponentModel;
+using System.Diagnostics;
+using System.IO;
+using System.Reflection;
+using System.Threading.Tasks;
+
+namespace Dynamo.Utilities
+{
+ ///
+ /// Base class for Dynamo CLI wrappers
+ ///
+ internal abstract class CLIWrapper : IDisposable
+ {
+ protected const string endOfDataToken = @"<<<<>>>>";
+ protected const string startofDataToken = @"<<<<>>>>";
+ protected readonly Process process = new Process();
+ protected bool started;
+ internal event Action MessageLogged;
+
+ public virtual void Dispose()
+ {
+ process.ErrorDataReceived -= Process_ErrorDataReceived;
+ KillProcess();
+ }
+
+ ///
+ /// Start the process.
+ ///
+ /// relative path to the exe to start.
+ /// argument string to pass to process.
+ protected virtual void StartProcess(string relativeEXEPath, string argString)
+ {
+ ProcessStartInfo startInfo = new ProcessStartInfo
+ {
+ CreateNoWindow = true,
+ RedirectStandardOutput = true,
+ RedirectStandardInput = true,
+ RedirectStandardError = true,
+
+ UseShellExecute = false,
+ Arguments = argString,
+ FileName = GetToolPath(relativeEXEPath)
+ };
+
+ process.StartInfo = startInfo;
+ try
+ {
+ process.Start();
+ started = true;
+ //the only purpose here is to avoid deadlocks when std err gets filled up 4kb
+ //in long running processes.
+ process.ErrorDataReceived += Process_ErrorDataReceived;
+ process.BeginErrorReadLine();
+
+ }
+ catch (Win32Exception)
+ {
+ // Do nothing
+ }
+ }
+
+ private void Process_ErrorDataReceived(object sender, DataReceivedEventArgs e)
+ {
+ //do nothing, we just want to empty the error stream.
+ }
+
+
+
+ ///
+ /// Kill the CLI tool - if running
+ ///
+ protected void KillProcess()
+ {
+ if (started)
+ {
+ if (!process.HasExited)
+ {
+ process.Kill();
+ }
+ started = false;
+ }
+ process.Dispose();
+ }
+ ///
+ /// Compute the location of the CLI tool.
+ ///
+ /// Returns full path to the CLI tool
+ protected static string GetToolPath(string relativePath)
+ {
+ var rootPath = Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location) ?? throw new ArgumentNullException(nameof(Path.GetDirectoryName));
+ var toolPath = Path.Combine(rootPath,relativePath);
+ return toolPath;
+ }
+ ///
+ /// Read data from CLI tool
+ ///
+ /// will return empty string if we don't finish reading all data in the timeout provided in milliseconds.
+ ///
+ protected virtual async Task GetData(int timeoutms)
+ {
+ var readStdOutTask = Task.Run(() =>
+ {
+ if (process.HasExited)
+ {
+ return string.Empty;
+ }
+
+ using (var writer = new StringWriter())
+ {
+ var done = false;
+ var start = false;
+ while (!done)
+ {
+ try
+ {
+ var line = process.StandardOutput.ReadLine();
+ MessageLogged?.Invoke(line);
+ if (line == null || line == startofDataToken)
+ {
+ start = true;
+ continue; //don't record start token to stream.
+ }
+
+ if (line == null || line == endOfDataToken)
+ {
+ done = true;
+ }
+ else
+ {
+ //if we have started recieving valid data, start recording
+ if (!string.IsNullOrWhiteSpace(line) && start)
+ {
+ writer.WriteLine(line);
+ }
+ }
+ }
+ catch (Exception)
+ {
+ KillProcess();
+ return GetCantCommunicateErrorMessage();
+ }
+ }
+
+ return writer.ToString();
+ }
+ });
+ var completedTask = await Task.WhenAny(readStdOutTask, Task.Delay(TimeSpan.FromMilliseconds(timeoutms)));
+ //if the completed task was our read std out task, then return the data
+ //else we timed out, so return an empty string.
+ return completedTask == readStdOutTask ? readStdOutTask.Result : string.Empty;
+ }
+
+ protected void RaiseMessageLogged(string message)
+ {
+ MessageLogged?.Invoke(message);
+ }
+
+ ///
+ /// Can't start Error message
+ ///
+ /// Returns error message
+ protected abstract string GetCantStartErrorMessage();
+
+
+ ///
+ /// Can't communicate Error message
+ ///
+ /// Returns error message
+ protected abstract string GetCantCommunicateErrorMessage();
+
+
+
+ }
+}
diff --git a/src/DynamoUtilities/DynamoFeatureFlagsManager.cs b/src/DynamoUtilities/DynamoFeatureFlagsManager.cs
index e4237cbd4b7..d60c886c7c9 100644
--- a/src/DynamoUtilities/DynamoFeatureFlagsManager.cs
+++ b/src/DynamoUtilities/DynamoFeatureFlagsManager.cs
@@ -24,6 +24,16 @@ internal class DynamoFeatureFlagsManager : CLIWrapper
private Dictionary AllFlagsCache { get; set; }//TODO lock is likely overkill.
private SynchronizationContext syncContext;
internal static event Action FlagsRetrieved;
+
+ //TODO(DYN-6464)- remove this field!.
+ ///
+ /// set to true after some FF issue is logged. For now we only log once to avoid clients overwhelming the logger.
+ ///
+ private bool loggedFFIssueOnce = false;
+ ///
+ /// Timeout in ms for feature flag communication with CLI process.
+ ///
+ private const int featureFlagTimeoutMs = 5000;
///
/// Constructor
@@ -54,18 +64,18 @@ internal void CacheAllFlags()
{
//wait for response
- var dataFromCLI = GetData();
+ var dataFromCLI = GetData(featureFlagTimeoutMs).Result;
//convert from json string to dictionary.
try
- {
+ {
AllFlagsCache = JsonConvert.DeserializeObject>(dataFromCLI);
//invoke the flags retrieved event on the sync context which should be the main ui thread.
syncContext?.Send((_) =>
- {
+ {
FlagsRetrieved?.Invoke();
- },null);
-
+ }, null);
+
}
catch (Exception e)
{
@@ -74,34 +84,45 @@ internal void CacheAllFlags()
}
///
- /// Check feature flag value, if not exist, return defaultval
+ /// Check feature flag value, if it does not exist, return the defaultval.
///
- ///
- ///
- ///
+ /// Must be a bool or string, only bool or string flags should be created unless this implementation is improved.
+ /// feature flag name
+ /// Currently the flag and default val MUST be a bool or string.
///
internal T CheckFeatureFlag(string featureFlagKey, T defaultval)
{
if(!(defaultval is bool || defaultval is string)){
- RaiseMessageLogged("unsupported flag type");
- return defaultval;
+ throw new ArgumentException("unsupported flag type", defaultval.GetType().ToString());
}
// if we have not retrieved flags from the cli return empty
- // and log.
+ // and log once.
if(AllFlagsCache == null)
- {
- RaiseMessageLogged("the flags cache is null, something went wrong retrieving feature flags," +
- " or you need to wait longer for the cache to populate, you can use the static FlagsRetrieved event for this purpose. ");
+ { //TODO(DYN-6464) Revisit this and log more when the logger is not easily overwhelmed.
+ if (!loggedFFIssueOnce)
+ {
+ RaiseMessageLogged(
+ $"The flags cache was null while checking {featureFlagKey}, something went wrong retrieving feature flags," +
+ " or you need to wait longer for the cache to populate before checking for flags, you can use the static FlagsRetrieved event for this purpose." +
+ "This message will not be logged again, and future calls to CheckFeatureFlags will return default values!!!");
+ }
+
+ loggedFFIssueOnce = true;
return defaultval;
}
- if (AllFlagsCache.ContainsKey(featureFlagKey))
+ if (AllFlagsCache.TryGetValue(featureFlagKey, out var flagVal))
{
- return (T)AllFlagsCache[featureFlagKey];
+ return (T)flagVal;
}
else
{
- RaiseMessageLogged($"failed to get value for feature flag key ex: {featureFlagKey},{System.Environment.NewLine} returning default value: {defaultval}");
+ if (!loggedFFIssueOnce)
+ {
+ RaiseMessageLogged(
+ $"failed to get value for feature flag key ex: {featureFlagKey},{System.Environment.NewLine} returning default value: {defaultval}");
+ }
+ loggedFFIssueOnce = true;
return defaultval;
}
}
diff --git a/src/DynamoUtilities/Md2Html.cs b/src/DynamoUtilities/Md2Html.cs
index 5d8c4ed1dc7..ceb5908b2a7 100644
--- a/src/DynamoUtilities/Md2Html.cs
+++ b/src/DynamoUtilities/Md2Html.cs
@@ -1,168 +1,11 @@
using System;
-using System.ComponentModel;
-using System.Diagnostics;
using System.IO;
-using System.Reflection;
+using System.Threading;
using DynamoUtilities.Properties;
+using Newtonsoft.Json.Linq;
namespace Dynamo.Utilities
{
- //TODO move to new file.
- ///
- /// Base class for Dynamo CLI wrappers
- ///
- internal abstract class CLIWrapper : IDisposable
- {
- protected const string endOfDataToken = @"<<<<>>>>";
- protected const string startofDataToken = @"<<<<>>>>";
- protected readonly Process process = new Process();
- protected bool started;
- internal event Action MessageLogged;
-
- public virtual void Dispose()
- {
- process.ErrorDataReceived -= Process_ErrorDataReceived;
- KillProcess();
- }
-
- ///
- /// Start the process.
- ///
- /// relative path to the exe to start.
- /// argument string to pass to process.
- protected virtual void StartProcess(string relativeEXEPath, string argString)
- {
- ProcessStartInfo startInfo = new ProcessStartInfo
- {
- CreateNoWindow = true,
- RedirectStandardOutput = true,
- RedirectStandardInput = true,
- RedirectStandardError = true,
-
- UseShellExecute = false,
- Arguments = argString,
- FileName = GetToolPath(relativeEXEPath)
- };
-
- process.StartInfo = startInfo;
- try
- {
- process.Start();
- started = true;
- //the only purspose here is to avoid deadlocks when std err gets filled up 4kb
- //in long running processes.
- process.ErrorDataReceived += Process_ErrorDataReceived;
- process.BeginErrorReadLine();
-
- }
- catch (Win32Exception)
- {
- // Do nothing
- }
- }
-
- private void Process_ErrorDataReceived(object sender, DataReceivedEventArgs e)
- {
- //do nothing, we just want to empty the error stream.
- }
-
-
-
- ///
- /// Kill the CLI tool - if running
- ///
- protected void KillProcess()
- {
- if (started)
- {
- if (!process.HasExited)
- {
- process.Kill();
- }
- started = false;
- }
- process.Dispose();
- }
- ///
- /// Compute the location of the CLI tool.
- ///
- /// Returns full path to the CLI tool
- protected static string GetToolPath(string relativePath)
- {
- var rootPath = Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location) ?? throw new ArgumentNullException(nameof(Path.GetDirectoryName));
- var toolPath = Path.Combine(rootPath,relativePath);
- return toolPath;
- }
- //TODO if we see any issues with deadlocks we can try using a timeout on another thread.
- ///
- /// Read data from CLI tool
- /// Returns data read from CLI tool
- ///
- protected virtual string GetData()
- {
- if (process.HasExited)
- {
- return string.Empty;
- }
- using (var writer = new StringWriter())
- {
- var done = false;
- var start = false;
- while (!done)
- {
- try
- {
- var line = process.StandardOutput.ReadLine();
- MessageLogged?.Invoke(line);
- if (line == null || line == startofDataToken)
- {
- start = true;
- continue;//don't record start token to stream.
- }
- if (line == null || line == endOfDataToken)
- {
- done = true;
- }
- else
- { //if we have started recieving valid data, start recording
- if (!string.IsNullOrWhiteSpace(line) && start)
- {
- writer.WriteLine(line);
- }
- }
- }
- catch (Exception e)
- {
- KillProcess();
- return GetCantCommunicateErrorMessage();
- }
- }
-
- return writer.ToString();
- }
- }
-
- protected void RaiseMessageLogged(string message)
- {
- MessageLogged?.Invoke(message);
- }
-
- ///
- /// Can't start Error message
- ///
- /// Returns error message
- protected abstract string GetCantStartErrorMessage();
-
-
- ///
- /// Can't communicate Error message
- ///
- /// Returns error message
- protected abstract string GetCantCommunicateErrorMessage();
-
-
-
- }
///
/// Utilities for converting Markdown to html and for sanitizing html
/// The Md2Html command line tool is used for doing the actual conversion/santizing
@@ -175,6 +18,8 @@ protected void RaiseMessageLogged(string message)
internal class Md2Html : CLIWrapper
{
private string relativePath = Path.Combine(@"Md2Html", @"Md2Html.exe");
+ private int processCommunicationTimeoutms = 5000;
+
///
/// Constructor
/// Start the CLI tool and keep it around
@@ -228,9 +73,9 @@ internal string ParseMd2Html(string mdString, string mdPath)
return GetCantCommunicateErrorMessage();
}
- var output = GetData();
+ var output = GetData(processCommunicationTimeoutms);
- return output;
+ return output.Result;
}
///
@@ -257,9 +102,9 @@ internal string SanitizeHtml(string content)
return GetCantCommunicateErrorMessage();
}
- var output = GetData();
+ var output = GetData(processCommunicationTimeoutms);
- return output;
+ return output.Result;
}
///
diff --git a/src/DynamoUtilities/Properties/AssemblyInfo.cs b/src/DynamoUtilities/Properties/AssemblyInfo.cs
index c3981334ac4..67a7633b408 100644
--- a/src/DynamoUtilities/Properties/AssemblyInfo.cs
+++ b/src/DynamoUtilities/Properties/AssemblyInfo.cs
@@ -24,3 +24,5 @@
[assembly: InternalsVisibleTo("DynamoApplications")]
[assembly: InternalsVisibleTo("DynamoCLI")]
[assembly: InternalsVisibleTo("NodeDocumentationMarkdownGenerator")]
+[assembly: InternalsVisibleTo("DynamoUtilitiesTests")]
+
diff --git a/test/DynamoCoreTests/Logging/FeatureFlagTests.cs b/test/DynamoCoreTests/Logging/FeatureFlagTests.cs
index 968d6b712a2..7fd1698a6b8 100644
--- a/test/DynamoCoreTests/Logging/FeatureFlagTests.cs
+++ b/test/DynamoCoreTests/Logging/FeatureFlagTests.cs
@@ -2,6 +2,7 @@
using NUnit.Framework;
using System;
using System.Diagnostics;
+using System.Text.RegularExpressions;
using System.Threading;
namespace Dynamo.Tests.Logging
@@ -69,6 +70,42 @@ public void FeatureFlagsShouldMessageLoggedShouldContainAllLogs()
StringAssert.EndsWith("<<<<>>>>", log);
}
+ //TODO(DYN-6464) Revisit this and log more when the logger is not easily overwhelmed.
+ [Test]
+ public void FeatureFlagsShouldMessageLoggedShouldOnlyContainNullFlagErrorOnce()
+ {
+ var testflagsManager = new DynamoUtilities.DynamoFeatureFlagsManager("testkey", new SynchronizationContext(), true);
+ testflagsManager.MessageLogged += TestflagsManager_MessageLogged;
+ testflagsManager.CheckFeatureFlag("TestFlag2", "na");
+ testflagsManager.CheckFeatureFlag("TestFlag2", "na");
+ testflagsManager.CheckFeatureFlag("TestFlag2", "na");
+ testflagsManager.MessageLogged -= TestflagsManager_MessageLogged;
+ var matches = Regex.Matches(log, "wait longer for the cache").Count;
+ Assert.AreEqual(1,matches);
+ }
+ //TODO(DYN-6464) Revisit this and log more when the logger is not easily overwhelmed.
+ [Test]
+ public void FeatureFlagsShouldMessageLoggedShouldOnlyContainMissingFlagErrorOnce()
+ {
+ var testflagsManager = new DynamoUtilities.DynamoFeatureFlagsManager("testkey", new SynchronizationContext(), true);
+ testflagsManager.MessageLogged += TestflagsManager_MessageLogged;
+ testflagsManager.CacheAllFlags();
+ testflagsManager.CheckFeatureFlag("MissingFlag", "na");
+ testflagsManager.CheckFeatureFlag("MissingFlag", "na");
+ testflagsManager.CheckFeatureFlag("MissingFlag", "na");
+ testflagsManager.MessageLogged -= TestflagsManager_MessageLogged;
+ var matches = Regex.Matches(log, "failed to get value").Count;
+ Assert.AreEqual(1, matches);
+ }
+ [Test]
+ public void FeatureFlagsThrowsIfCheckIngNonSupportedType()
+ {
+ var testflagsManager = new DynamoUtilities.DynamoFeatureFlagsManager("testkey", new SynchronizationContext(), true);
+ Assert.Throws(() =>
+ {
+ testflagsManager.CheckFeatureFlag("NumericTypeNotSupported", 10);
+ });
+ }
private void DynamoFeatureFlagsManager_FlagsRetrieved()
{
diff --git a/test/Libraries/DynamoUtilitiesTests/CLIWrapperTests.cs b/test/Libraries/DynamoUtilitiesTests/CLIWrapperTests.cs
new file mode 100644
index 00000000000..0657e0b5a70
--- /dev/null
+++ b/test/Libraries/DynamoUtilitiesTests/CLIWrapperTests.cs
@@ -0,0 +1,58 @@
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Linq;
+using System.Text;
+using System.Threading.Tasks;
+using NUnit.Framework;
+
+namespace DynamoUtilitiesTests
+{
+ [TestFixture]
+ public class CLIWrapperTests
+ {
+ ///
+ /// A test class that starts up the DynamoFF CLI and then kills it to cause a deadlock.
+ ///
+ private class HangingCLIWrapper: Dynamo.Utilities.CLIWrapper
+ {
+ private string relativePath = Path.Combine("DynamoFeatureFlags", "DynamoFeatureFlags.exe");
+ protected override string GetCantStartErrorMessage()
+ {
+ throw new NotImplementedException();
+ }
+
+ protected override string GetCantCommunicateErrorMessage()
+ {
+ throw new NotImplementedException();
+ }
+ internal HangingCLIWrapper()
+ {
+ StartProcess(relativePath, null);
+ }
+
+ internal string GetData()
+ {
+ //wait a bit, and then kill the process
+ //this will cause GetData to hang and timeout.
+ Task.Run(() =>
+ { System.Threading.Thread.Sleep(100);
+ process.Kill();
+ });
+ return GetData(2000).Result;
+ }
+ }
+
+ [Test]
+ public void CLIWrapperDoesNotHangIfProcessDoesNotWriteToStdOut()
+ {
+ var sw = new System.Diagnostics.Stopwatch();
+ sw.Start();
+ var wrapper = new HangingCLIWrapper();
+ Assert.AreEqual(string.Empty,wrapper.GetData());
+ sw.Stop();
+ Assert.GreaterOrEqual(sw.ElapsedMilliseconds,2000);
+
+ }
+ }
+}