Skip to content

Fix critical memory leaks in disposal patterns#16857

Draft
Copilot wants to merge 7 commits intomasterfrom
copilot/fix-memory-leak-patterns
Draft

Fix critical memory leaks in disposal patterns#16857
Copilot wants to merge 7 commits intomasterfrom
copilot/fix-memory-leak-patterns

Conversation

Copy link
Contributor

Copilot AI commented Jan 28, 2026

Four critical memory leak patterns identified: undisposed Timer in PulseMaker, uncleared event handlers in IDSDKManager and WorkspaceModel, and uncleared collections in NodeModel. These prevent garbage collection in long-running sessions and workspace lifecycle operations.

Changes

PulseMaker - Timer disposal

Implemented IDisposable with thread-safe disposal:

internal class PulseMaker : IDisposable
{
    protected virtual void Dispose(bool disposing)
    {
        if (!disposed && disposing)
        {
            lock (stateMutex)  // Thread-safe with timer callbacks
            {
                TimerPeriod = 0;
                internalTimer.Change(Timeout.Infinite, Timeout.Infinite);
                RunStarted = null;
            }
            internalTimer.Dispose();
            disposed = true;
        }
    }
}

IDSDKManager - Event handler cleanup

Clear public events RequestLogin and LoginStateChanged in Dispose() to prevent subscriber references from persisting.

NodeModel - Collection cleanup

Clear InPorts, OutPorts, DismissedAlerts, inputNodes, outputNodes after disposing child objects:

public override void Dispose()
{
    if (HasBeenDisposed) return;
    base.Dispose();
    
    InPorts.CollectionChanged -= PortsCollectionChanged;
    foreach(var port in InPorts) DisposePort(port, nodeDisposing: true);
    InPorts.Clear();
    
    OutPorts.CollectionChanged -= PortsCollectionChanged;
    foreach(var port in OutPorts) DisposePort(port, nodeDisposing: true);
    OutPorts.Clear();
    
    DismissedAlerts?.Clear();
    inputNodes.Clear();
    outputNodes.Clear();
}

WorkspaceModel - Event and dictionary cleanup

Null 21 event handlers and clear 3 dictionaries (nodePackageDictionary, localDefinitionsDictionary, externalFilesDictionary) in Dispose().

ModelBase - Test accessibility

Changed HasBeenDisposed from protected to protected internal to enable test verification via InternalsVisibleTo.

Test Coverage

Added 25 unit tests across disposal scenarios: normal disposal, double-dispose safety, empty collections, thread safety, and connected node cleanup.

Original prompt

Dynamo in the past has history of memory leaks. Plan how to fix common memory leak patterns in this code base and wait for instruction.


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI changed the title [WIP] Plan to fix common memory leak patterns in Dynamo Memory Leak Remediation Plan - Analysis and Strategy Jan 28, 2026
Copilot AI requested a review from QilongTang January 28, 2026 11:26
Copilot AI and others added 3 commits January 28, 2026 13:40
…nager cleanup

Co-authored-by: QilongTang <3942418+QilongTang@users.noreply.github.com>
…l collections and events

Co-authored-by: QilongTang <3942418+QilongTang@users.noreply.github.com>
…bustness

Co-authored-by: QilongTang <3942418+QilongTang@users.noreply.github.com>
Copilot AI changed the title Memory Leak Remediation Plan - Analysis and Strategy Fix Phase 1 critical memory leaks: Timer, events, and collection disposal Jan 28, 2026
capturedObject = null;

// Force garbage collection
GC.Collect();
// Force garbage collection
GC.Collect();
GC.WaitForPendingFinalizers();
GC.Collect();
addNode = null;

// Force garbage collection
GC.Collect();
// Force garbage collection
GC.Collect();
GC.WaitForPendingFinalizers();
GC.Collect();
capturedObject = null;

// Force garbage collection
GC.Collect();
// Force garbage collection
GC.Collect();
GC.WaitForPendingFinalizers();
GC.Collect();
// Subscribe a handler that captures the object
var capturedObject = weakRef.Target;
manager.RequestLogin += (obj) => {
var _ = capturedObject; // Capture in closure
Assert.Greater(addNode.InPorts.Count, 0, "Node should have input ports");
Assert.Greater(addNode.OutPorts.Count, 0, "Node should have output ports");

var inPortCount = addNode.InPorts.Count;
Assert.Greater(addNode.OutPorts.Count, 0, "Node should have output ports");

var inPortCount = addNode.InPorts.Count;
var outPortCount = addNode.OutPorts.Count;
@QilongTang QilongTang added the WIP label Jan 28, 2026
@QilongTang
Copy link
Contributor

@copilot The build job failed primarily due to C# compilation errors related to missing namespaces from the test code. The key errors are:

error CS0234: The type or namespace name 'Workspaces' does not exist in the namespace 'Dynamo.Tests.Graph'
error CS0234: The type or namespace name 'Nodes' does not exist in the namespace 'Dynamo.Tests.Graph'
error CS0234: The type or namespace name 'ModelBase' does not exist in the namespace 'Dynamo.Tests.Graph'

Co-authored-by: QilongTang <3942418+QilongTang@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Jan 30, 2026

@copilot The build job failed primarily due to C# compilation errors related to missing namespaces from the test code. The key errors are:

error CS0234: The type or namespace name 'Workspaces' does...

Fixed in commit 695074d. The test files were using the incorrect namespace Dynamo.Tests.Graph which doesn't exist. Changed to Dynamo.Tests to match the existing test file conventions in the repository.

…null'

Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
addNode.Dispose();

// Clear the node reference
addNode = null;
Co-authored-by: QilongTang <3942418+QilongTang@users.noreply.github.com>
Copilot AI changed the title Fix Phase 1 critical memory leaks: Timer, events, and collection disposal Make HasBeenDisposed accessible to test assemblies Jan 30, 2026
Copilot AI changed the title Make HasBeenDisposed accessible to test assemblies Fix critical memory leaks in disposal patterns Feb 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants