diff --git a/src/Microsoft.TestPlatform.Common/DataCollection/DataCollectionAttachmentManager.cs b/src/Microsoft.TestPlatform.Common/DataCollection/DataCollectionAttachmentManager.cs index a952366b1d..5599c615b8 100644 --- a/src/Microsoft.TestPlatform.Common/DataCollection/DataCollectionAttachmentManager.cs +++ b/src/Microsoft.TestPlatform.Common/DataCollection/DataCollectionAttachmentManager.cs @@ -4,6 +4,7 @@ namespace Microsoft.VisualStudio.TestPlatform.Common.DataCollector; using System; +using System.Collections.Concurrent; using System.Collections.Generic; using System.ComponentModel; using System.Diagnostics; @@ -14,18 +15,31 @@ namespace Microsoft.VisualStudio.TestPlatform.Common.DataCollector; using System.Threading.Tasks; using Interfaces; -using ObjectModel; + using Microsoft.VisualStudio.TestPlatform.ObjectModel.DataCollection; using Microsoft.VisualStudio.TestPlatform.ObjectModel.Logging; using Microsoft.VisualStudio.TestPlatform.Utilities; using Microsoft.VisualStudio.TestPlatform.Utilities.Helpers.Interfaces; +using ObjectModel; + /// /// Manages file transfer from data collector to test runner service. +/// +/// Events are handled sequentially so it's not possible have parallel AddAttachment/GetAttachments for the same DataCollectionContext. +/// DataCollectionContext can be a session context(session start/end) or a test case context(test case start/end). +/// +/// We have two type of events that will fire a collection of files "session end" and "test case end". +/// File are sent and copied/moved in parallel using async tasks, for these reason we need to use an async structure ConcurrentDictionary +/// to be able to handle parallel test case start/end events(if host run tests in parallel). +/// +/// We could avoid to use ConcurrentDictionary for the list of the attachment sets of a specific DataCollectionContext, but +/// we don't know how the user will implement the datacollector and they could send file out of events(wrong usage, no more expected sequential access AddAttachment->GetAttachments), +/// so we prefer protect every collection. This not means that outcome will be "always correct"(file attached in a correct way) but at least we avoid exceptions. /// internal class DataCollectionAttachmentManager : IDataCollectionAttachmentManager { - private static readonly object AttachmentTaskLock = new(); + private readonly object _attachmentTaskLock = new(); #region Fields @@ -42,7 +56,7 @@ internal class DataCollectionAttachmentManager : IDataCollectionAttachmentManage /// /// Attachment transfer tasks associated with a given datacollection context. /// - private readonly Dictionary> _attachmentTasks; + private readonly ConcurrentDictionary> _attachmentTasks; /// /// Use to cancel attachment transfers if test run is canceled. @@ -74,8 +88,8 @@ protected DataCollectionAttachmentManager(IFileHelper fileHelper) { _fileHelper = fileHelper; _cancellationTokenSource = new CancellationTokenSource(); - _attachmentTasks = new Dictionary>(); - AttachmentSets = new Dictionary>(); + _attachmentTasks = new ConcurrentDictionary>(); + AttachmentSets = new ConcurrentDictionary>(); } #endregion @@ -90,7 +104,7 @@ protected DataCollectionAttachmentManager(IFileHelper fileHelper) /// /// Gets the attachment sets for the given datacollection context. /// - internal Dictionary> AttachmentSets + internal ConcurrentDictionary> AttachmentSets { get; private set; } @@ -155,8 +169,8 @@ public List GetAttachments(DataCollectionContext dataCollectionCo if (AttachmentSets.TryGetValue(dataCollectionContext, out var uriAttachmentSetMap)) { attachments = uriAttachmentSetMap.Values.ToList(); - _attachmentTasks.Remove(dataCollectionContext); - AttachmentSets.Remove(dataCollectionContext); + _attachmentTasks.TryRemove(dataCollectionContext, out _); + AttachmentSets.TryRemove(dataCollectionContext, out _); } return attachments; @@ -180,14 +194,14 @@ public void AddAttachment(FileTransferInformation fileTransferInfo, AsyncComplet if (!AttachmentSets.ContainsKey(fileTransferInfo.Context)) { - var uriAttachmentSetMap = new Dictionary(); - AttachmentSets.Add(fileTransferInfo.Context, uriAttachmentSetMap); - _attachmentTasks.Add(fileTransferInfo.Context, new List()); + var uriAttachmentSetMap = new ConcurrentDictionary(); + AttachmentSets.TryAdd(fileTransferInfo.Context, uriAttachmentSetMap); + _attachmentTasks.TryAdd(fileTransferInfo.Context, new List()); } if (!AttachmentSets[fileTransferInfo.Context].ContainsKey(uri)) { - AttachmentSets[fileTransferInfo.Context].Add(uri, new AttachmentSet(uri, friendlyName)); + AttachmentSets[fileTransferInfo.Context].TryAdd(uri, new AttachmentSet(uri, friendlyName)); } AddNewFileTransfer(fileTransferInfo, sendFileCompletedCallback, uri, friendlyName); @@ -327,7 +341,7 @@ private void AddNewFileTransfer(FileTransferInformation fileTransferInfo, AsyncC { if (t.Exception == null) { - lock (AttachmentTaskLock) + lock (_attachmentTaskLock) { AttachmentSets[fileTransferInfo.Context][uri].Attachments.Add(UriDataAttachment.CreateFrom(localFilePath, fileTransferInfo.Description)); } diff --git a/src/Microsoft.TestPlatform.Common/DataCollection/DataCollectionManager.cs b/src/Microsoft.TestPlatform.Common/DataCollection/DataCollectionManager.cs index 8bde074f51..694527f2e9 100644 --- a/src/Microsoft.TestPlatform.Common/DataCollection/DataCollectionManager.cs +++ b/src/Microsoft.TestPlatform.Common/DataCollection/DataCollectionManager.cs @@ -768,10 +768,28 @@ private void RemoveDataCollectors(IReadOnlyCollection private void LogAttachments(List attachmentSets) { + if (attachmentSets is null) + { + EqtTrace.Error("DataCollectionManager.LogAttachments: Unexpected null attachmentSets."); + return; + } + foreach (var entry in attachmentSets) { + if (entry is null) + { + EqtTrace.Error("DataCollectionManager.LogAttachments: Unexpected null entry inside attachmentSets."); + continue; + } + foreach (var file in entry.Attachments) { + if (file is null) + { + EqtTrace.Error("DataCollectionManager.LogAttachments: Unexpected null file inside entry attachments."); + continue; + } + EqtTrace.Verbose( "Test Attachment Description: Collector:'{0}' Uri:'{1}' Description:'{2}' Uri:'{3}' ", entry.DisplayName, diff --git a/src/Microsoft.TestPlatform.CommunicationUtilities/DataCollectionRequestHandler.cs b/src/Microsoft.TestPlatform.CommunicationUtilities/DataCollectionRequestHandler.cs index a8d0401335..9e2cd01e20 100644 --- a/src/Microsoft.TestPlatform.CommunicationUtilities/DataCollectionRequestHandler.cs +++ b/src/Microsoft.TestPlatform.CommunicationUtilities/DataCollectionRequestHandler.cs @@ -76,7 +76,7 @@ protected DataCollectionRequestHandler(IMessageSink messageSink, IRequestData re new SocketCommunicationManager(), messageSink, DataCollectionManager.Create(messageSink, requestData), - new DataCollectionTestCaseEventHandler(), + new DataCollectionTestCaseEventHandler(messageSink), JsonDataSerializer.Instance, new FileHelper(), requestData) @@ -162,7 +162,7 @@ public static DataCollectionRequestHandler Create( communicationManager, messageSink, DataCollectionManager.Create(messageSink, requestData), - new DataCollectionTestCaseEventHandler(), + new DataCollectionTestCaseEventHandler(messageSink), JsonDataSerializer.Instance, new FileHelper(), requestData); @@ -362,7 +362,7 @@ private void HandleBeforeTestRunStart(Message message) } catch (Exception e) { - EqtTrace.Error("DataCollectionRequestHandler.HandleBeforeTestRunStart : Error occurred during initialization of TestHost : {0}", e); + EqtTrace.Error("DataCollectionRequestHandler.HandleBeforeTestRunStart : Error occurred during test case events handling: {0}.", e); } }, _cancellationTokenSource.Token); diff --git a/src/Microsoft.TestPlatform.CommunicationUtilities/DataCollectionTestCaseEventHandler.cs b/src/Microsoft.TestPlatform.CommunicationUtilities/DataCollectionTestCaseEventHandler.cs index ab60d76a0c..7adad505b9 100644 --- a/src/Microsoft.TestPlatform.CommunicationUtilities/DataCollectionTestCaseEventHandler.cs +++ b/src/Microsoft.TestPlatform.CommunicationUtilities/DataCollectionTestCaseEventHandler.cs @@ -3,14 +3,19 @@ namespace Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.DataCollection; +using System; +using System.Collections.ObjectModel; using System.Net; using Common.DataCollector; + using Microsoft.VisualStudio.TestPlatform.Common.DataCollector.Interfaces; using Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.Interfaces; -using ObjectModel; using Microsoft.VisualStudio.TestPlatform.ObjectModel; using Microsoft.VisualStudio.TestPlatform.ObjectModel.DataCollection; +using Microsoft.VisualStudio.TestPlatform.ObjectModel.Logging; + +using ObjectModel; /// /// The test case data collection request handler. @@ -20,14 +25,14 @@ internal class DataCollectionTestCaseEventHandler : IDataCollectionTestCaseEvent private readonly ICommunicationManager _communicationManager; private readonly IDataCollectionManager _dataCollectionManager; private readonly IDataSerializer _dataSerializer; + private readonly IMessageSink _messageSink; /// /// Initializes a new instance of the class. /// - internal DataCollectionTestCaseEventHandler() - : this(new SocketCommunicationManager(), DataCollectionManager.Instance, JsonDataSerializer.Instance) - { - } + internal DataCollectionTestCaseEventHandler(IMessageSink messageSink) + : this(messageSink, new SocketCommunicationManager(), DataCollectionManager.Instance, JsonDataSerializer.Instance) + { } /// /// Initializes a new instance of the class. @@ -35,11 +40,12 @@ internal DataCollectionTestCaseEventHandler() /// Communication manager implementation. /// Data collection manager implementation. /// Serializer for serialization and deserialization of the messages. - internal DataCollectionTestCaseEventHandler(ICommunicationManager communicationManager, IDataCollectionManager dataCollectionManager, IDataSerializer dataSerializer) + internal DataCollectionTestCaseEventHandler(IMessageSink messageSink, ICommunicationManager communicationManager, IDataCollectionManager dataCollectionManager, IDataSerializer dataSerializer) { _communicationManager = communicationManager; _dataCollectionManager = dataCollectionManager; _dataSerializer = dataSerializer; + _messageSink = messageSink; } /// @@ -79,7 +85,17 @@ public void ProcessRequests() } var testCaseStartEventArgs = _dataSerializer.DeserializePayload(message); - _dataCollectionManager.TestCaseStarted(testCaseStartEventArgs); + + try + { + _dataCollectionManager.TestCaseStarted(testCaseStartEventArgs); + } + catch (Exception ex) + { + _messageSink.SendMessage(new DataCollectionMessageEventArgs(TestMessageLevel.Error, $"Error occurred during TestCaseStarted event handling: {ex}")); + EqtTrace.Error($"DataCollectionTestCaseEventHandler.ProcessRequests: Error occurred during TestCaseStarted event handling: {ex}"); + } + _communicationManager.SendMessage(MessageType.DataCollectionTestStartAck); if (EqtTrace.IsInfoEnabled) @@ -96,7 +112,19 @@ public void ProcessRequests() } var testCaseEndEventArgs = _dataSerializer.DeserializePayload(message); - var attachmentSets = _dataCollectionManager.TestCaseEnded(testCaseEndEventArgs); + + Collection attachmentSets; + try + { + attachmentSets = _dataCollectionManager.TestCaseEnded(testCaseEndEventArgs); + } + catch (Exception ex) + { + _messageSink.SendMessage(new DataCollectionMessageEventArgs(TestMessageLevel.Error, $"Error occurred during DataCollectionTestEnd event handling: {ex}")); + EqtTrace.Error($"DataCollectionTestCaseEventHandler.ProcessRequests: Error occurred during DataCollectionTestEnd event handling: {ex}"); + attachmentSets = new Collection(); + } + _communicationManager.SendMessage(MessageType.DataCollectionTestEndResult, attachmentSets); if (EqtTrace.IsInfoEnabled) @@ -114,7 +142,15 @@ public void ProcessRequests() EqtTrace.Info("DataCollectionTestCaseEventHandler: Test session ended"); } - Close(); + try + { + Close(); + } + catch (Exception ex) + { + _messageSink.SendMessage(new DataCollectionMessageEventArgs(TestMessageLevel.Error, $"Error occurred during SessionEnd event handling: {ex}")); + EqtTrace.Error($"DataCollectionTestCaseEventHandler.ProcessRequests: Error occurred during SessionEnd event handling: {ex}"); + } break; diff --git a/test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/DataCollectionTestCaseEventHandlerTests.cs b/test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/DataCollectionTestCaseEventHandlerTests.cs index 67b87d8afb..e8550fb1e1 100644 --- a/test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/DataCollectionTestCaseEventHandlerTests.cs +++ b/test/Microsoft.TestPlatform.CommunicationUtilities.UnitTests/DataCollectionTestCaseEventHandlerTests.cs @@ -27,13 +27,15 @@ public class DataCollectionTestCaseEventHandlerTests private readonly Mock _mockDataCollectionManager; private readonly DataCollectionTestCaseEventHandler _requestHandler; private readonly Mock _dataSerializer; + private readonly Mock _messageSink; public DataCollectionTestCaseEventHandlerTests() { _mockCommunicationManager = new Mock(); _mockDataCollectionManager = new Mock(); _dataSerializer = new Mock(); - _requestHandler = new DataCollectionTestCaseEventHandler(_mockCommunicationManager.Object, new Mock().Object, _dataSerializer.Object); + _messageSink = new Mock(); + _requestHandler = new DataCollectionTestCaseEventHandler(_messageSink.Object, _mockCommunicationManager.Object, new Mock().Object, _dataSerializer.Object); } [TestMethod] @@ -91,7 +93,7 @@ public void CloseShouldThrowExceptionIfThrownByCommunicationManager() [TestMethod] public void CloseShouldNotThrowExceptionIfCommunicationManagerIsNull() { - var requestHandler = new DataCollectionTestCaseEventHandler(null, new Mock().Object, _dataSerializer.Object); + var requestHandler = new DataCollectionTestCaseEventHandler(_messageSink.Object, null, new Mock().Object, _dataSerializer.Object); requestHandler.Close(); @@ -107,7 +109,7 @@ public void ProcessRequestsShouldProcessBeforeTestCaseStartEvent() _mockCommunicationManager.SetupSequence(x => x.ReceiveMessage()).Returns(message).Returns(new Message() { MessageType = MessageType.SessionEnd, Payload = "false" }); - var requestHandler = new DataCollectionTestCaseEventHandler(_mockCommunicationManager.Object, _mockDataCollectionManager.Object, _dataSerializer.Object); + var requestHandler = new DataCollectionTestCaseEventHandler(_messageSink.Object, _mockCommunicationManager.Object, _mockDataCollectionManager.Object, _dataSerializer.Object); requestHandler.ProcessRequests(); @@ -124,7 +126,7 @@ public void ProcessRequestsShouldProcessAfterTestCaseCompleteEvent() _mockCommunicationManager.SetupSequence(x => x.ReceiveMessage()).Returns(message).Returns(new Message() { MessageType = MessageType.SessionEnd, Payload = "false" }); - var requestHandler = new DataCollectionTestCaseEventHandler(_mockCommunicationManager.Object, _mockDataCollectionManager.Object, _dataSerializer.Object); + var requestHandler = new DataCollectionTestCaseEventHandler(_messageSink.Object, _mockCommunicationManager.Object, _mockDataCollectionManager.Object, _dataSerializer.Object); requestHandler.ProcessRequests(); diff --git a/test/datacollector.UnitTests/DataCollectionAttachmentManagerTests.cs b/test/datacollector.UnitTests/DataCollectionAttachmentManagerTests.cs index 7045ef1cf0..45b74e5355 100644 --- a/test/datacollector.UnitTests/DataCollectionAttachmentManagerTests.cs +++ b/test/datacollector.UnitTests/DataCollectionAttachmentManagerTests.cs @@ -4,17 +4,22 @@ namespace Microsoft.VisualStudio.TestPlatform.Common.DataCollector.UnitTests; using System; +using System.Collections.Generic; using System.ComponentModel; using System.IO; using System.Threading; +using System.Threading.Tasks; using Interfaces; + +using Microsoft.VisualStudio.TestPlatform.ObjectModel; using Microsoft.VisualStudio.TestPlatform.ObjectModel.DataCollection; using Microsoft.VisualStudio.TestPlatform.Utilities.Helpers.Interfaces; -using TestTools.UnitTesting; using Moq; +using TestTools.UnitTesting; + [TestClass] public class DataCollectionAttachmentManagerTests { @@ -39,6 +44,56 @@ public void Cleanup() File.Delete(Path.Combine(TempDirectoryPath, "filename1.txt")); } + [TestMethod] + public void ParallelAccessShouldNotBreak() + { + string outputDirectory = Path.Combine(TempDirectoryPath, Guid.NewGuid().ToString()); + var dataCollectorSessionId = new SessionId(Guid.NewGuid()); + + try + { + _attachmentManager.Initialize(dataCollectorSessionId, outputDirectory, _messageSink.Object); + + CancellationTokenSource cts = new(TimeSpan.FromSeconds(3)); + List parallelTasks = new(); + int totalTasks = 3; + + // 3 tasks are enough to break bugged code + for (int i = 0; i < totalTasks; i++) + { + parallelTasks.Add(Task.Run(() => + { + while (true) + { + if (cts.IsCancellationRequested) + { + break; + } + _ = TestCaseEvent($"test_{Guid.NewGuid()}"); + } + })); + } + + Task.WaitAll(parallelTasks.ToArray()); + } + finally + { + if (Directory.Exists(outputDirectory)) + { + Directory.Delete(outputDirectory, true); + } + } + + List TestCaseEvent(string uri) + { + var testCaseCtx = new DataCollectionContext(dataCollectorSessionId, new TestExecId(Guid.NewGuid())); + string path = Path.Combine(outputDirectory, Guid.NewGuid().ToString()); + File.WriteAllText(path, "test"); + _attachmentManager.AddAttachment(new FileTransferInformation(testCaseCtx, path, true), null, new Uri($"//{uri}"), $"{uri}"); + return _attachmentManager.GetAttachments(testCaseCtx); + } + } + [TestMethod] public void InitializeShouldThrowExceptionIfSessionIdIsNull() {