Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] async code and dispose in tests #14649

Closed
wants to merge 15 commits into from
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
using System.Diagnostics;
using System.IO;
using System.Reflection;
using System.Threading.Tasks;
using System.Web;
using System.Windows;
using System.Windows.Controls;
Expand All @@ -21,7 +22,8 @@ public partial class DocumentationBrowserView : UserControl, IDisposable
private readonly DocumentationBrowserViewModel viewModel;
private const string VIRTUAL_FOLDER_MAPPING = "appassets";
static readonly string HTML_IMAGE_PATH_PREFIX = @"http://";
private bool hasBeenInitialized;
private Task hasBeenInitialized;
private bool isDisposing;
private ScriptingObject comScriptingObject;
private string fontStylePath = "Dynamo.Wpf.Views.GuidedTour.HtmlPages.Resources.ArtifaktElement-Regular.woff";

Expand Down Expand Up @@ -104,32 +106,30 @@ private void ShouldAllowNavigation(object sender, CoreWebView2NavigationStarting
/// <param name="link"></param>
public void NavigateToPage(Uri link)
{
InitializeAsync();
Dispatcher.BeginInvoke(InitializeAsync);
}

protected virtual void Dispose(bool disposing)
{
// Cleanup
this.viewModel.LinkChanged -= NavigateToPage;
this.documentationBrowser.NavigationStarting -= ShouldAllowNavigation;
this.documentationBrowser.DpiChanged -= DocumentationBrowser_DpiChanged;

if (this.documentationBrowser.CoreWebView2 != null)
if (this.documentationBrowser != null)
{
this.documentationBrowser.CoreWebView2.WebMessageReceived -= CoreWebView2OnWebMessageReceived;
}
this.documentationBrowser.NavigationStarting -= ShouldAllowNavigation;
this.documentationBrowser.DpiChanged -= DocumentationBrowser_DpiChanged;

// Note to test writers
// Disposing the document browser will cause future tests
// that uses the Browser component to crash
if (!Models.DynamoModel.IsTestMode)
{
if (this.documentationBrowser.CoreWebView2 != null)
{
this.documentationBrowser.CoreWebView2.WebMessageReceived -= CoreWebView2OnWebMessageReceived;
}
this.documentationBrowser.Dispose();
}
}

async void InitializeAsync()
{
if (isDisposing) return;

VirtualFolderPath = string.Empty;
try
{
Expand Down Expand Up @@ -159,7 +159,7 @@ async void InitializeAsync()
}

// Only initialize once
if (!hasBeenInitialized)
if (hasBeenInitialized == null)
{
if (!string.IsNullOrEmpty(WebBrowserUserDataFolder))
{
Expand All @@ -170,32 +170,30 @@ async void InitializeAsync()
};
}
//Initialize the CoreWebView2 component otherwise we can't navigate to a web page
await documentationBrowser.EnsureCoreWebView2Async();


this.documentationBrowser.CoreWebView2.WebMessageReceived += CoreWebView2OnWebMessageReceived;
comScriptingObject = new ScriptingObject(this.viewModel);
//register the interop object into the browser.
this.documentationBrowser.CoreWebView2.AddHostObjectToScript("bridge", comScriptingObject);

this.documentationBrowser.CoreWebView2.Settings.IsZoomControlEnabled = true;
this.documentationBrowser.CoreWebView2.Settings.AreDevToolsEnabled = true;

hasBeenInitialized = true;
hasBeenInitialized = documentationBrowser.EnsureCoreWebView2Async().ContinueWith((_) => {
if (isDisposing) return;

this.documentationBrowser.CoreWebView2.WebMessageReceived += CoreWebView2OnWebMessageReceived;
comScriptingObject = new ScriptingObject(this.viewModel);
//register the interop object into the browser.
this.documentationBrowser.CoreWebView2.AddHostObjectToScript("bridge", comScriptingObject);

this.documentationBrowser.CoreWebView2.Settings.IsZoomControlEnabled = true;
this.documentationBrowser.CoreWebView2.Settings.AreDevToolsEnabled = true;
}, TaskScheduler.FromCurrentSynchronizationContext());
}
await hasBeenInitialized;
if (isDisposing) return;

if(Directory.Exists(VirtualFolderPath))
if (Directory.Exists(VirtualFolderPath))
//Due that the Web Browser(WebView2 - Chromium) security CORS is blocking the load of resources like images then we need to create a virtual folder in which the image are located.
this.documentationBrowser.CoreWebView2.SetVirtualHostNameToFolderMapping(VIRTUAL_FOLDER_MAPPING, VirtualFolderPath, CoreWebView2HostResourceAccessKind.DenyCors);

string htmlContent = this.viewModel.GetContent();

htmlContent = ResourceUtilities.LoadResourceAndReplaceByKey(htmlContent, "#fontStyle", fontStylePath);

Dispatcher.BeginInvoke(new Action(() =>
{
this.documentationBrowser.NavigateToString(htmlContent);
}));
this.documentationBrowser.NavigateToString(htmlContent);
}

private void CoreWebView2OnWebMessageReceived(object sender, CoreWebView2WebMessageReceivedEventArgs e)
Expand All @@ -207,8 +205,17 @@ private void CoreWebView2OnWebMessageReceived(object sender, CoreWebView2WebMess
/// <summary>
/// Dispose function for DocumentationBrowser
/// </summary>
public void Dispose()
public async void Dispose()
{
isDisposing = true;
if (Models.DynamoModel.IsTestMode && hasBeenInitialized != null)
{
GC.SuppressFinalize(this);
await hasBeenInitialized;
Dispose(true);
return;
}

Dispose(true);
GC.SuppressFinalize(this);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ public override void Loaded(ViewLoadedParams viewLoadedParams)

public override void Shutdown()
{
Dispose();
// Do nothing for now
}

private void OnInsertFile(object sender, InsertDocumentationLinkEventArgs e)
Expand Down
5 changes: 3 additions & 2 deletions src/DynamoUtilities/CLIWrapper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ protected static string GetToolPath(string relativePath)
/// </summary>
/// <param name="timeoutms">will return empty string if we don't finish reading all data in the timeout provided in milliseconds.</param>
/// <returns></returns>
protected virtual async Task<string> GetData(int timeoutms)
protected virtual string GetData(int timeoutms)
{
var readStdOutTask = Task.Run(() =>
{
Expand Down Expand Up @@ -145,7 +145,8 @@ 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;
Expand Down
2 changes: 1 addition & 1 deletion src/DynamoUtilities/DynamoFeatureFlagsManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down
6 changes: 2 additions & 4 deletions src/DynamoUtilities/Md2Html.cs
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,7 @@ internal string ParseMd2Html(string mdString, string mdPath)
return GetCantCommunicateErrorMessage();
}

var output = GetData(processCommunicationTimeoutms);

return output.Result;
return GetData(processCommunicationTimeoutms);
}

/// <summary>
Expand Down Expand Up @@ -104,7 +102,7 @@ internal string SanitizeHtml(string content)

var output = GetData(processCommunicationTimeoutms);

return output.Result;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This caused a deadlock when running tests on a single thread (could happen in live scenario too)
The .Result blocks the current thread until the task is completed.
And inside the GetData() the await Task will never finish because the main thread cannot poll for the status

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would an alternative be to always access this data from another thread, for example - like we do with the feature flags manager startup - run the GetData call inside on a task in the thread pool?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might work. But having a separate thread started everywhere GetData.Result is called might be harder to maintain
I find it easier to reason about when the entire concurrency logic is boxed up inside the GetData method so callers do not need to worry about how to call it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually used the approach you suggested for another case (in a test file)
https://github.com/DynamoDS/Dynamo/pull/14649/files#diff-be236d5cf3315a8f37d3d67a4adb628825430289b534dbd620952c47f2d286b0R1136
For production code (like GetData) I still think it would make it tricky to figure out how to use it safely.

return output;
}

/// <summary>
Expand Down
42 changes: 32 additions & 10 deletions src/LibraryViewExtensionWebView2/LibraryViewController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
using Dynamo.Models;
using Dynamo.Search;
using Dynamo.Search.SearchElements;
using Dynamo.Utilities;
using Dynamo.ViewModels;
using Dynamo.Wpf.Interfaces;
using Dynamo.Wpf.UI.GuidedTour;
Expand Down Expand Up @@ -73,6 +74,8 @@ public class LibraryViewController : IDisposable
private ICommandExecutive commandExecutive;
private DynamoViewModel dynamoViewModel;
private FloatingLibraryTooltipPopup libraryViewTooltip;
private Task isInitialized;
bool isDisposing;
// private ResourceHandlerFactory resourceFactory;
private IDisposable observer;
internal WebView2 browser;
Expand Down Expand Up @@ -319,9 +322,6 @@ internal void AddLibraryView()
browser.Loaded += Browser_Loaded;
browser.SizeChanged += Browser_SizeChanged;

this.browser = view.mainGrid.Children.OfType<WebView2>().FirstOrDefault();
InitializeAsync();

LibraryViewController.SetupSearchModelEventsObserver(browser, dynamoViewModel.Model.SearchModel,
this, this.customization);
}
Expand Down Expand Up @@ -350,12 +350,19 @@ async void InitializeAsync()
};
}

await browser.EnsureCoreWebView2Async();
this.browser.CoreWebView2.WebMessageReceived += CoreWebView2_WebMessageReceived;
twoWayScriptingObject = new ScriptingObject(this);
//register the interop object into the browser.
this.browser.CoreWebView2.AddHostObjectToScript("bridgeTwoWay", twoWayScriptingObject);
browser.CoreWebView2.Settings.IsZoomControlEnabled = true;
isInitialized = browser.EnsureCoreWebView2Async().ContinueWith((_) =>
{
if (isDisposing) return;

this.browser.CoreWebView2.WebMessageReceived += CoreWebView2_WebMessageReceived;
twoWayScriptingObject = new ScriptingObject(this);
//register the interop object into the browser.
this.browser.CoreWebView2.AddHostObjectToScript("bridgeTwoWay", twoWayScriptingObject);
browser.CoreWebView2.Settings.IsZoomControlEnabled = true;
}, TaskScheduler.FromCurrentSynchronizationContext());
await isInitialized;

if (isDisposing) return;
}

private void CoreWebView2_WebMessageReceived(object sender, CoreWebView2WebMessageReceivedEventArgs args)
Expand Down Expand Up @@ -422,6 +429,11 @@ private void Browser_Loaded(object sender, RoutedEventArgs e)
{
string msg = "Browser Loaded";
LogToDynamoConsole(msg);

if (!isDisposing)
{
InitializeAsync();
}
}

// This enum is for matching the modifier keys between C# and javaScript
Expand Down Expand Up @@ -712,8 +724,18 @@ internal void LogToDynamoConsole(string message)
this.dynamoViewModel.Model.Logger.Log(message);
}

public void Dispose()
public async void Dispose()
{
isDisposing = true;

if (Models.DynamoModel.IsTestMode && isInitialized != null)
{
GC.SuppressFinalize(this);
await isInitialized;
Dispose(true);
return;
}

Dispose(true);
GC.SuppressFinalize(this);
}
Expand Down
Loading
Loading