fix more memory leaks when starting and closing DynamoRevit #14366
Conversation
…ynamoDS#14344) * fixing mem leaks in progress * fix test and remove todo
get rid of redudant try/catch cleanup log event in docs browser extension replace item produced lambda with named method fix broken dispose logic in nodeautocompletesearchcontrol - it was always hanging onto static event implement dispose on viewextensioncommandexec on dynamoviewmodel dispose, also dispose all workspaces - their remove methods are not called... fix entry added lambda in searchviewmodel dynamoview now disposes workspace view and node autocomplete search - TODO here. workspaceview implement idisposable and cleans up view model subscriptions direct manip extension was not cleaning up settings prop changed. graph meta data view was not cleaning up current workspace cleared event attempt to cleanup browser com refs... not sure this is helping
| @@ -47,7 +47,7 @@ public NodeAutoCompleteSearchControl() | |||
| Application.Current.Deactivated += CurrentApplicationDeactivated; | |||
| if (Application.Current.MainWindow != null) | |||
| { | |||
| Application.Current.MainWindow.Closing -= NodeAutoCompleteSearchControl_Unloaded; | |||
| Application.Current.MainWindow.Closing += NodeAutoCompleteSearchControl_Unloaded; | |||
There was a problem hiding this comment.
Nice catch @RobertGlobant20 @mjkkirschner
| @@ -403,6 +403,7 @@ public override void Dispose() | |||
| { | |||
| cate.DisposeTree(); | |||
| } | |||
| Model.EntryAdded += AddEntry; | |||
There was a problem hiding this comment.
Shouldn't this be -=?
|
Thanks for the catch - will send a fix.On Sep 5, 2023, at 9:35 PM, aparajit-pratap ***@***.***> wrote:
@aparajit-pratap commented on this pull request.
In src/DynamoCoreWpf/ViewModels/Search/SearchViewModel.cs:
@@ -403,6 +403,7 @@ public override void Dispose()
{
cate.DisposeTree();
}
+ Model.EntryAdded += AddEntry;
Shouldn't this be -=?
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you modified the open/close state.Message ID: ***@***.***>
|
| //TODO code smell. | ||
| var workspaceView = this.ChildOfType<WorkspaceView>(); | ||
| workspaceView?.Dispose(); | ||
| (workspaceView?.NodeAutoCompleteSearchBar?.Child as IDisposable)?.Dispose(); |
There was a problem hiding this comment.
Why do we need to dispose this specific control?
There was a problem hiding this comment.
the NodeAutoCompleteSearchControl has a dispose method which does not seem to be called from anywhere else.
There was a problem hiding this comment.
Minor point but how about disposing it inside the workspaceview's dispose method - would be cleaner.
| var settings = GetRunSettings(WorkspaceModel); | ||
| if (settings != null) | ||
| { | ||
| settings.PropertyChanged -= OnRunSettingsPropertyChanged; |
There was a problem hiding this comment.
Why does need to be unsubscribed here?
There was a problem hiding this comment.
Because when Dynamo is shutdown the SetWorkspaceModel call above is not called. So we do it somewhere in the shutdown/dispose hook path of this extension.
In a context like Revit where Dynamo can be shutdown and started over and over, this leads to leaks. In Sandbox it's not an issue.
| @@ -266,6 +266,7 @@ private void MarkCurrentWorkspaceModified() | |||
| public void Dispose() | |||
| { | |||
| this.viewLoadedParams.CurrentWorkspaceChanged -= OnCurrentWorkspaceChanged; | |||
| this.viewLoadedParams.CurrentWorkspaceCleared += OnCurrentWorkspaceChanged; | |||
There was a problem hiding this comment.
Are you sure it's not -=?
There was a problem hiding this comment.
hmm, it's probably a copy paste error, but I will double check and add it to #14379
Purpose
This PR fixes more memory leaks when starting and closing DynamoRevit and creating a single new workspace each time.
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
fix additional memory leaks.
Reviewers