From 7f15942dc7a2490cc12712f4896ba718e6788bff Mon Sep 17 00:00:00 2001 From: Michael Kirschner Date: Wed, 20 Mar 2024 15:55:31 -0400 Subject: [PATCH] cherry pick CLI deadlock fix to 2.19.5 (#15030) master-15 has some flaky tests, but they were fixed in master so I will merge this. --- .../Views/SplashScreen/SplashScreen.xaml.cs | 1 + src/DynamoUtilities/CLIWrapper.cs | 17 ++++- .../DynamoFeatureFlagsManager.cs | 2 +- src/DynamoUtilities/Md2Html.cs | 8 +-- .../DynamoUtilitiesTests/CLIWrapperTests.cs | 64 ++++++++++++++++++- 5 files changed, 81 insertions(+), 11 deletions(-) diff --git a/src/DynamoCoreWpf/Views/SplashScreen/SplashScreen.xaml.cs b/src/DynamoCoreWpf/Views/SplashScreen/SplashScreen.xaml.cs index 2f547539fbe..824aad876f2 100644 --- a/src/DynamoCoreWpf/Views/SplashScreen/SplashScreen.xaml.cs +++ b/src/DynamoCoreWpf/Views/SplashScreen/SplashScreen.xaml.cs @@ -511,6 +511,7 @@ protected override void OnClosed(EventArgs e) DynamoModel.RequestUpdateLoadBarStatus -= DynamoModel_RequestUpdateLoadBarStatus; DynamoModel.LanguageDetected -= DynamoModel_LanguageDetected; + StaticSplashScreenReady -= OnStaticScreenReady; webView.Dispose(); webView = null; diff --git a/src/DynamoUtilities/CLIWrapper.cs b/src/DynamoUtilities/CLIWrapper.cs index 5578e233867..70e71d2f8e0 100644 --- 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 /// /// will return empty string if we don't finish reading all data in the timeout provided in milliseconds. + /// if this delegate is non null, it will be used instead of communicating with std out of the process. Used for testing only. /// - protected virtual async Task GetData(int timeoutms) + protected virtual string GetData(int timeoutms, Func mockReadLine = null) { var readStdOutTask = Task.Run(() => { @@ -114,7 +115,17 @@ protected virtual async Task 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 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 index d60c886c7c9..de0614bedff 100644 --- 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 index ceb5908b2a7..c7115826667 100644 --- 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); } /// @@ -102,9 +100,7 @@ internal string SanitizeHtml(string content) return GetCantCommunicateErrorMessage(); } - var output = GetData(processCommunicationTimeoutms); - - return output.Result; + return GetData(processCommunicationTimeoutms); } /// diff --git a/test/Libraries/DynamoUtilitiesTests/CLIWrapperTests.cs b/test/Libraries/DynamoUtilitiesTests/CLIWrapperTests.cs index 0657e0b5a70..b34a814f198 100644 --- 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); + } + + + } + /// + /// this test class waits before reading from the console, so GetData is slow. + /// + private class SlowCLIWrapper : HangingCLIWrapper + { + internal new string GetData() + { + return GetData(2000, () => { Thread.Sleep(4000);return ""; }); + } + } + + /// + /// this test class should get mock data and should not time out. + /// + 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); + + } } }