Skip to content

Commit

Permalink
Address review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@Evangelink
Evangelink committed Jul 15, 2022
1 parent 59e3921 commit 51e97c7
Show file tree
Hide file tree
Showing 21 changed files with 182 additions and 140 deletions.
36 changes: 23 additions & 13 deletions ...taCollectors/Microsoft.TestPlatform.Extensions.EventLogCollector/EventLogDataCollector.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions src/Microsoft.TestPlatform.Client/DesignMode/DesignModeClient.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}

Expand Down
32 changes: 11 additions & 21 deletions src/Microsoft.TestPlatform.Client/Discovery/DiscoveryRequest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public void DiscoverAsync()

lock (_syncObject)
{
if (_disposed)
if (_isDisposed)
{
throw new ObjectDisposedException(nameof(DiscoveryRequest));
}
Expand Down Expand Up @@ -110,7 +110,7 @@ public void Abort()

lock (_syncObject)
{
if (_disposed)
if (_isDisposed)
{
throw new ObjectDisposedException(nameof(DiscoveryRequest));
}
Expand Down Expand Up @@ -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");
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
}
}

Expand All @@ -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.
Expand Down
20 changes: 10 additions & 10 deletions src/Microsoft.TestPlatform.Client/Execution/TestRunRequest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -108,7 +108,7 @@ public int ExecuteAsync()

lock (_syncObject)
{
if (_disposed)
if (_isDisposed)
{
throw new ObjectDisposedException("testRunRequest");
}
Expand Down Expand Up @@ -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");
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -684,7 +684,7 @@ protected virtual void Dispose(bool disposing)

lock (_syncObject)
{
if (!_disposed)
if (!_isDisposed)
{
if (disposing)
{
Expand All @@ -693,7 +693,7 @@ protected virtual void Dispose(bool disposing)

// Indicate that object has been disposed
_runCompletionEvent = null!;
_disposed = true;
_isDisposed = true;
}
}

Expand Down
6 changes: 3 additions & 3 deletions src/Microsoft.TestPlatform.Common/DataCollection/DataCollectionManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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;
}
}

Expand Down
30 changes: 12 additions & 18 deletions src/Microsoft.TestPlatform.CommunicationUtilities/SocketTransport.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
3 changes: 0 additions & 3 deletions src/Microsoft.TestPlatform.CrossPlatEngine/Adapter/FrameworkHandle.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
2 changes: 1 addition & 1 deletion src/Microsoft.TestPlatform.CrossPlatEngine/Client/TestLoggerManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down
6 changes: 6 additions & 0 deletions src/Microsoft.TestPlatform.CrossPlatEngine/EventHandlers/TestRequestHandler.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ public class TestRequestHandler : ITestRequestHandler, IDeploymentAwareTestReque
private Action<Message>? _onAttachDebuggerAckRecieved;
private IPathConverter _pathConverter;
private Exception? _messageProcessingUnrecoverableError;
private bool _isDisposed;

public TestHostConnectionInfo ConnectionInfo { get; set; }
string? IDeploymentAwareTestRequestHandler.LocalPath { get; set; }
Expand Down Expand Up @@ -161,11 +162,16 @@ public void Dispose()

protected virtual void Dispose(bool disposing)
{
if (_isDisposed)
return;

if (disposing)
{
_communicationEndPoint?.Stop();
_channel?.Dispose();
}

_isDisposed = true;
}

/// <inheritdoc />
Expand Down
Loading

0 comments on commit 51e97c7

Please sign in to comment.