Enforce use of correct dispose pattern

.editorconfig

@@ -225,6 +225,9 @@ dotnet_diagnostic.CA1305.severity = warning # not default, increased severity to
 # CA1822: Mark members as static
 dotnet_diagnostic.CA1822.severity = warning # not default, increased severity to ensure it is applied
 
+# CA1816: Dispose methods should call SuppressFinalize
+dotnet_diagnostic.CA1816.severity = warning # not default, increased severity to ensure it is applied
+
 #### C# Coding Conventions ####
 
 # var preferences

src/DataCollectors/Microsoft.TestPlatform.Extensions.EventLogCollector/EventLogDataCollector.cs

@@ -91,6 +91,8 @@ public class EventLogDataCollector : DataCollector
     /// </summary>
     private readonly IDictionary<string, IEventLogContainer> _eventLogContainerMap = new Dictionary<string, IEventLogContainer>();
 
+    private bool _isDisposed;
+
     /// <summary>
     /// Initializes a new instance of the <see cref="EventLogDataCollector"/> class.
     /// </summary>
@@ -290,25 +292,33 @@ internal string WriteEventLogs(List<EventLogEntry> eventLogEntries, int maxLogEn
     /// <param name="disposing">Not used since this class does not have a finalizer.</param>
     protected override void Dispose(bool disposing)
     {
+        if (_isDisposed)
+            return;
+
         base.Dispose(disposing);
 
-        // Unregister events
-        if (_events != null)
+        if (disposing)
         {
-            _events.SessionStart -= _sessionStartEventHandler;
-            _events.SessionEnd -= _sessionEndEventHandler;
-            _events.TestCaseStart -= _testCaseStartEventHandler;
-            _events.TestCaseEnd -= _testCaseEndEventHandler;
-        }
+            // Unregister events
+            if (_events != null)
+            {
+                _events.SessionStart -= _sessionStartEventHandler;
+                _events.SessionEnd -= _sessionEndEventHandler;
+                _events.TestCaseStart -= _testCaseStartEventHandler;
+                _events.TestCaseEnd -= _testCaseEndEventHandler;
+            }
 
-        // Unregister EventLogEntry Written.
-        foreach (var eventLogContainer in _eventLogContainerMap.Values)
-        {
-            eventLogContainer.Dispose();
+            // Unregister EventLogEntry Written.
+            foreach (var eventLogContainer in _eventLogContainerMap.Values)
+            {
+                eventLogContainer.Dispose();
+            }
+
+            // Delete all the temp event log directories
+            RemoveTempEventLogDirs(_eventLogDirectories);
         }
 
-        // Delete all the temp event log directories
-        RemoveTempEventLogDirs(_eventLogDirectories);
+        _isDisposed = true;
     }
 
     #endregion

src/Microsoft.TestPlatform.Client/DesignMode/DesignModeClient.cs

@@ -642,18 +642,18 @@ void OnError(TestSessionEventsHandler eventsHandler, Exception? ex)
 
     #region IDisposable Support
 
-    private bool _disposedValue; // To detect redundant calls
+    private bool _isDisposed; // To detect redundant calls
 
     protected virtual void Dispose(bool disposing)
     {
-        if (!_disposedValue)
+        if (!_isDisposed)
         {
             if (disposing)
             {
-                _communicationManager?.StopClient();
+                _communicationManager.StopClient();
             }
 
-            _disposedValue = true;
+            _isDisposed = true;
         }
     }
 
@@ -662,6 +662,7 @@ public void Dispose()
     {
         // Do not change this code. Put cleanup code in Dispose(bool disposing) above.
         Dispose(true);
+        GC.SuppressFinalize(this);
     }
     #endregion
 }

src/Microsoft.TestPlatform.Client/Discovery/DiscoveryRequest.cs

@@ -68,7 +68,7 @@ public void DiscoverAsync()
 
         lock (_syncObject)
         {
-            if (_disposed)
+            if (_isDisposed)
             {
                 throw new ObjectDisposedException(nameof(DiscoveryRequest));
             }
@@ -110,7 +110,7 @@ public void Abort()
 
         lock (_syncObject)
         {
-            if (_disposed)
+            if (_isDisposed)
             {
                 throw new ObjectDisposedException(nameof(DiscoveryRequest));
             }
@@ -149,7 +149,7 @@ bool IRequest.WaitForCompletion(int timeout)
     {
         EqtTrace.Verbose("DiscoveryRequest.WaitForCompletion: Waiting with timeout {0}.", timeout);
 
-        if (_disposed)
+        if (_isDisposed)
         {
             throw new ObjectDisposedException("DiscoveryRequest");
         }
@@ -221,7 +221,7 @@ public void HandleDiscoveryComplete(DiscoveryCompleteEventArgs discoveryComplete
 
         lock (_syncObject)
         {
-            if (_disposed)
+            if (_isDisposed)
             {
                 EqtTrace.Warning("DiscoveryRequest.HandleDiscoveryComplete: Ignoring as the object is disposed.");
                 return;
@@ -311,7 +311,7 @@ public void HandleDiscoveredTests(IEnumerable<TestCase>? discoveredTestCases)
 
         lock (_syncObject)
         {
-            if (_disposed)
+            if (_isDisposed)
             {
                 EqtTrace.Warning("DiscoveryRequest.HandleDiscoveredTests: Ignoring as the object is disposed.");
                 return;
@@ -336,7 +336,7 @@ public void HandleLogMessage(TestMessageLevel level, string? message)
 
         lock (_syncObject)
         {
-            if (_disposed)
+            if (_isDisposed)
             {
                 EqtTrace.Warning("DiscoveryRequest.HandleLogMessage: Ignoring as the object is disposed.");
                 return;
@@ -476,31 +476,21 @@ private void HandleLoggerManagerDiscoveryComplete(DiscoveryCompletePayload? disc
     /// resetting unmanaged resources.
     /// </summary>
     public void Dispose()
-    {
-        Dispose(true);
-
-        GC.SuppressFinalize(this);
-    }
-
-    private void Dispose(bool disposing)
     {
         EqtTrace.Verbose("DiscoveryRequest.Dispose: Starting.");
 
         lock (_syncObject)
         {
-            if (!_disposed)
+            if (!_isDisposed)
             {
-                if (disposing)
+                if (_discoveryCompleted != null)
                 {
-                    if (_discoveryCompleted != null)
-                    {
-                        _discoveryCompleted.Dispose();
-                    }
+                    _discoveryCompleted.Dispose();
                 }
 
                 // Indicate that object has been disposed
                 _discoveryCompleted = null!;
-                _disposed = true;
+                _isDisposed = true;
             }
         }
 
@@ -516,7 +506,7 @@ private void Dispose(bool disposing)
     /// <summary>
     /// If this request has been disposed.
     /// </summary>
-    private bool _disposed;
+    private bool _isDisposed;
 
     /// <summary>
     /// It get set when current discovery request is completed.

src/Microsoft.TestPlatform.Client/Execution/TestRunRequest.cs

@@ -33,7 +33,7 @@ public class TestRunRequest : ITestRunRequest, IInternalTestRunEventsHandler
     /// <summary>
     /// Specifies whether the run is disposed or not
     /// </summary>
-    private bool _disposed;
+    private bool _isDisposed;
 
     /// <summary>
     /// Sync object for various operations
@@ -108,7 +108,7 @@ public int ExecuteAsync()
 
         lock (_syncObject)
         {
-            if (_disposed)
+            if (_isDisposed)
             {
                 throw new ObjectDisposedException("testRunRequest");
             }
@@ -186,7 +186,7 @@ public bool WaitForCompletion(int timeout)
     {
         EqtTrace.Verbose("TestRunRequest.WaitForCompletion: Waiting with timeout {0}.", timeout);
 
-        if (_disposed)
+        if (_isDisposed)
         {
             throw new ObjectDisposedException("testRunRequest");
         }
@@ -215,7 +215,7 @@ public void CancelAsync()
 
         lock (_cancelSyncObject)
         {
-            if (_disposed)
+            if (_isDisposed)
             {
                 EqtTrace.Warning("Ignoring TestRunRequest.CancelAsync() as testRunRequest object has already been disposed.");
                 return;
@@ -244,7 +244,7 @@ public void Abort()
 
         lock (_cancelSyncObject)
         {
-            if (_disposed)
+            if (_isDisposed)
             {
                 EqtTrace.Warning("Ignoring TestRunRequest.Abort() as testRunRequest object has already been disposed");
                 return;
@@ -365,7 +365,7 @@ public void HandleTestRunComplete(TestRunCompleteEventArgs runCompleteArgs, Test
         lock (_syncObject)
         {
             // If this object is disposed, don't do anything
-            if (_disposed)
+            if (_isDisposed)
             {
                 EqtTrace.Warning("TestRunRequest.TestRunComplete: Ignoring as the object is disposed.");
                 return;
@@ -481,7 +481,7 @@ public virtual void HandleTestRunStatsChange(TestRunChangedEventArgs? testRunCha
         lock (_syncObject)
         {
             // If this object is disposed, don't do anything
-            if (_disposed)
+            if (_isDisposed)
             {
                 EqtTrace.Warning("TestRunRequest.SendTestRunStatsChange: Ignoring as the object is disposed.");
                 return;
@@ -506,7 +506,7 @@ public void HandleLogMessage(TestMessageLevel level, string? message)
         lock (_syncObject)
         {
             // If this object is disposed, don't do anything
-            if (_disposed)
+            if (_isDisposed)
             {
                 EqtTrace.Warning("TestRunRequest.SendTestRunMessage: Ignoring as the object is disposed.");
                 return;
@@ -684,7 +684,7 @@ protected virtual void Dispose(bool disposing)
 
         lock (_syncObject)
         {
-            if (!_disposed)
+            if (!_isDisposed)
             {
                 if (disposing)
                 {
@@ -693,7 +693,7 @@ protected virtual void Dispose(bool disposing)
 
                 // Indicate that object has been disposed
                 _runCompletionEvent = null!;
-                _disposed = true;
+                _isDisposed = true;
             }
         }
 

src/Microsoft.TestPlatform.Common/DataCollection/DataCollectionManager.cs

@@ -55,7 +55,7 @@ internal class DataCollectionManager : IDataCollectionManager
     /// <summary>
     /// Specifies whether the object is disposed or not.
     /// </summary>
-    private bool _disposed;
+    private bool _isDisposed;
 
     /// <summary>
     /// Extension manager for data collectors.
@@ -360,14 +360,14 @@ public Collection<AttachmentSet> TestCaseEnded(TestCaseEndEventArgs testCaseEndE
     /// </param>
     protected virtual void Dispose(bool disposing)
     {
-        if (!_disposed)
+        if (!_isDisposed)
         {
             if (disposing)
             {
                 CleanupPlugins();
             }
 
-            _disposed = true;
+            _isDisposed = true;
         }
     }
 

src/Microsoft.TestPlatform.CommunicationUtilities/SocketTransport.cs

@@ -61,27 +61,21 @@ public bool WaitForConnection(int connectionTimeout)
 
     public void Dispose()
     {
-        Dispose(true);
-    }
-
-    private void Dispose(bool disposing)
-    {
-        if (!_disposed)
+        if (_disposed)
         {
-            if (disposing)
-            {
-                if (_connectionInfo.Role == ConnectionRole.Client)
-                {
-                    _communicationManager?.StopClient();
-                }
-                else
-                {
-                    _communicationManager?.StopServer();
-                }
-            }
+            return;
+        }
 
-            _disposed = true;
+        if (_connectionInfo.Role == ConnectionRole.Client)
+        {
+            _communicationManager.StopClient();
         }
+        else
+        {
+            _communicationManager.StopServer();
+        }
+
+        _disposed = true;
     }
 
     /// <summary>

src/Microsoft.TestPlatform.CoreUtilities/PublicAPI/PublicAPI.Shipped.txt

@@ -86,6 +86,7 @@ Microsoft.VisualStudio.TestPlatform.Utilities.IOutput.Write(string? message, Mic
 Microsoft.VisualStudio.TestPlatform.Utilities.IOutput.WriteLine(string? message, Microsoft.VisualStudio.TestPlatform.Utilities.OutputLevel level) -> void
 Microsoft.VisualStudio.TestPlatform.Utilities.JobQueue<T>
 Microsoft.VisualStudio.TestPlatform.Utilities.JobQueue<T>.Dispose() -> void
+virtual Microsoft.VisualStudio.TestPlatform.Utilities.JobQueue<T>.Dispose(bool disposing) -> void
 Microsoft.VisualStudio.TestPlatform.Utilities.JobQueue<T>.Flush() -> void
 Microsoft.VisualStudio.TestPlatform.Utilities.JobQueue<T>.JobQueue(System.Action<T?>! processJob, string! displayName, int maxQueueLength, int maxQueueSize, bool enableBounds, System.Action<string!>! exceptionLogger) -> void
 Microsoft.VisualStudio.TestPlatform.Utilities.JobQueue<T>.Pause() -> void

src/Microsoft.TestPlatform.CoreUtilities/Utilities/JobQueue.cs

@@ -197,7 +197,14 @@ public void Flush()
     /// </summary>
     public void Dispose()
     {
-        if (_isDisposed)
+        Dispose(true);
+        GC.SuppressFinalize(this);
+    }
+
+    protected virtual void Dispose(bool disposing)
+    {
+        if (_isDisposed
+            || !disposing)
         {
             return;
         }

src/Microsoft.TestPlatform.CrossPlatEngine/Adapter/FrameworkHandle.cs

@@ -117,9 +117,6 @@ public bool AttachDebuggerToProcess(int pid)
     public void Dispose()
     {
         Dispose(true);
-
-        // Use SupressFinalize in case a subclass
-        // of this valueType implements a finalizer.
         GC.SuppressFinalize(this);
     }
 

src/Microsoft.TestPlatform.CrossPlatEngine/Client/TestLoggerManager.cs

@@ -501,7 +501,7 @@ internal void EnableLogging()
     /// <param name="disposing">
     /// The disposing.
     /// </param>
-    internal virtual void Dispose(bool disposing)
+    protected virtual void Dispose(bool disposing)
     {
         if (!_isDisposed)
         {