cherry pick CLIWrapper deadlock fix (#14662)

mjkkirschner · web-flow · commit b9ebacaa0560 · 2023-12-01T09:57:59.000-05:00
* Tibi's changes

* review comments

* fix test
diff --git a/src/DynamoCoreWpf/Views/SplashScreen/SplashScreen.xaml.cs b/src/DynamoCoreWpf/Views/SplashScreen/SplashScreen.xaml.cs
@@ -546,7 +546,10 @@ protected override void OnClosed(EventArgs e)
             DynamoModel.RequestUpdateLoadBarStatus -= DynamoModel_RequestUpdateLoadBarStatus;
             DynamoModel.LanguageDetected -= DynamoModel_LanguageDetected;
             StaticSplashScreenReady -= OnStaticScreenReady;
-            authManager.LoginStateChanged -= OnLoginStateChanged;
+            if (authManager is not null)
+            {
+                authManager.LoginStateChanged -= OnLoginStateChanged;
+            }
             webView.Dispose();
             webView = null;
 
diff --git a/src/DynamoUtilities/CLIWrapper.cs b/src/DynamoUtilities/CLIWrapper.cs
@@ -96,8 +96,9 @@ protected static string GetToolPath(string relativePath)
         /// Read data from CLI tool
         /// </summary>
         /// <param name="timeoutms">will return empty string if we don't finish reading all data in the timeout provided in milliseconds.</param>
+        /// <param name="mockReadLine"> if this delegate is non null, it will be used instead of communicating with std out of the process. Used for testing only.</param>
         /// <returns></returns>
-        protected virtual async Task<string> GetData(int timeoutms)
+        protected virtual string GetData(int timeoutms, Func<string> mockReadLine = null)
         {
             var readStdOutTask = Task.Run(() =>
             {
@@ -114,7 +115,17 @@ protected virtual async Task<string> GetData(int timeoutms)
                     {
                         try
                         {
-                            var line = process.StandardOutput.ReadLine();
+                            string line = null;
+                            if(mockReadLine != null)
+                            {
+                                line = mockReadLine.Invoke();
+                            }
+                            else
+                            {
+                                line = process.StandardOutput.ReadLine();
+                            }
+                            
+                           
                             MessageLogged?.Invoke(line);
                             if (line == null || line == startofDataToken)
                             {
@@ -145,7 +156,7 @@ protected virtual async Task<string> GetData(int timeoutms)
                     return writer.ToString();
                 }
             });
-            var completedTask = await Task.WhenAny(readStdOutTask, Task.Delay(TimeSpan.FromMilliseconds(timeoutms)));
+            var completedTask = Task.WhenAny(readStdOutTask, Task.Delay(TimeSpan.FromMilliseconds(timeoutms))).Result;
             //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;
diff --git a/src/DynamoUtilities/DynamoFeatureFlagsManager.cs b/src/DynamoUtilities/DynamoFeatureFlagsManager.cs
@@ -64,7 +64,7 @@ internal void CacheAllFlags()
         {
 
             //wait for response
-            var dataFromCLI = GetData(featureFlagTimeoutMs).Result;    
+            var dataFromCLI = GetData(featureFlagTimeoutMs);    
             //convert from json string to dictionary.
             try
             {
diff --git a/src/DynamoUtilities/Md2Html.cs b/src/DynamoUtilities/Md2Html.cs
@@ -73,9 +73,7 @@ internal string ParseMd2Html(string mdString, string mdPath)
                 return GetCantCommunicateErrorMessage();
             }
 
-            var output = GetData(processCommunicationTimeoutms);
-
-            return output.Result;
+            return GetData(processCommunicationTimeoutms);
         }
 
         /// <summary>
@@ -102,9 +100,7 @@ internal string SanitizeHtml(string content)
                 return GetCantCommunicateErrorMessage();
             }
 
-            var output = GetData(processCommunicationTimeoutms);
-
-           return output.Result;
+            return GetData(processCommunicationTimeoutms);
         }
 
         /// <summary>
diff --git a/test/Libraries/DynamoUtilitiesTests/CLIWrapperTests.cs b/test/Libraries/DynamoUtilitiesTests/CLIWrapperTests.cs
@@ -3,6 +3,7 @@
 using System.IO;
 using System.Linq;
 using System.Text;
+using System.Threading;
 using System.Threading.Tasks;
 using NUnit.Framework;
 
@@ -39,7 +40,46 @@ internal string GetData()
                 {   System.Threading.Thread.Sleep(100);
                     process.Kill();
                 });
-                return GetData(2000).Result;
+                return GetData(2000);
+            }
+
+
+        }
+        /// <summary>
+        /// this test class waits before reading from the console, so GetData is slow.
+        /// </summary>
+        private class SlowCLIWrapper : HangingCLIWrapper
+        {
+            internal new string GetData()
+            {
+                return GetData(2000, () => { Thread.Sleep(4000);return ""; });
+            }
+        }
+
+        /// <summary>
+        /// this test class should get mock data and should not time out.
+        /// </summary>
+        private class MockCLIWraper : HangingCLIWrapper
+        {
+            int count = 0;
+            internal new string GetData()
+            {
+                return GetData(2000, () => {
+                    count++;
+
+                    switch (count)
+                    {
+                        case 1:
+                            return startofDataToken;
+                        case 2:
+                            return "some data";
+                        case 3:
+                            return endOfDataToken;
+                        default:
+                            return string.Empty;
+                    }
+                    
+                });
             }
         }
 
@@ -54,5 +94,27 @@ public void CLIWrapperDoesNotHangIfProcessDoesNotWriteToStdOut()
             Assert.GreaterOrEqual(sw.ElapsedMilliseconds,2000);
 
         }
+        [Test]
+        public void CLIWrapperTimesOutIfGetDataIsSlow()
+        {
+            var sw = new System.Diagnostics.Stopwatch();
+            sw.Start();
+            var wrapper = new SlowCLIWrapper();
+            Assert.AreEqual(string.Empty, wrapper.GetData());
+            sw.Stop();
+            Assert.GreaterOrEqual(sw.ElapsedMilliseconds, 2000);
+
+        }
+        [Test]
+        public void CLIGetsDataIfDoesNotTimeout()
+        {
+            var sw = new System.Diagnostics.Stopwatch();
+            sw.Start();
+            var wrapper = new MockCLIWraper();
+            Assert.AreEqual("some data", wrapper.GetData().TrimEnd());
+            sw.Stop();
+            Assert.LessOrEqual(sw.ElapsedMilliseconds, 2000);
+
+        }
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -64,7 +64,7 @@ internal void CacheAllFlags()`
`64`	`64`	`{`
`65`	`65`
`66`	`66`	`//wait for response`
`67`		`- var dataFromCLI = GetData(featureFlagTimeoutMs).Result;`
	`67`	`+ var dataFromCLI = GetData(featureFlagTimeoutMs);`
`68`	`68`	`//convert from json string to dictionary.`
`69`	`69`	`try`
`70`	`70`	`{`
Original file line number	Diff line number	Diff line change
`@@ -73,9 +73,7 @@ internal string ParseMd2Html(string mdString, string mdPath)`
`73`	`73`	`return GetCantCommunicateErrorMessage();`
`74`	`74`	`}`
`75`	`75`
`76`		`- var output = GetData(processCommunicationTimeoutms);`
`77`		`-`
`78`		`- return output.Result;`
	`76`	`+ return GetData(processCommunicationTimeoutms);`
`79`	`77`	`}`
`80`	`78`
`81`	`79`	`/// <summary>`
`@@ -102,9 +100,7 @@ internal string SanitizeHtml(string content)`
`102`	`100`	`return GetCantCommunicateErrorMessage();`
`103`	`101`	`}`
`104`	`102`
`105`		`- var output = GetData(processCommunicationTimeoutms);`
`106`		`-`
`107`		`- return output.Result;`
	`103`	`+ return GetData(processCommunicationTimeoutms);`
`108`	`104`	`}`
`109`	`105`
`110`	`106`	`/// <summary>`