Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix race condition inside DataCollectionAttachmentManager #3296

Merged
merged 1 commit into from
Jan 31, 2022

Conversation

MarcoRossignoli
Copy link
Contributor

@MarcoRossignoli MarcoRossignoli commented Jan 30, 2022

  • We're analyzing a strange race condition issue, but if we enable diag logs we're hiding real issue for an NRE inside the logs self.

image

  • Update Dictionary to ConcurrentDictionary to make DataCollectionAttachmentManager thread safe, concurrent call to the data collector TestCaseEnded will eventually fail.
 [TestMethod]
    public void ParallelAccess()
    {
        _attachmentManager.Initialize(new SessionId(Guid.NewGuid()), Path.GetTempPath(), _messageSink.Object);
        Task.WaitAll(

        Task.Run(() =>
        {
            try
            {
                while (true)
                {
                    var dc = new DataCollectionContext(new SessionId(Guid.NewGuid()));
                    string path = Path.GetTempFileName();
                    File.WriteAllText(path, "test");
                    _attachmentManager.AddAttachment(new FileTransferInformation(dc, path, true), null, new Uri("//test1"), "test1");
                    var res = _attachmentManager.GetAttachments(dc);
                }
            }
            catch (Exception ex)
            {
                Assert.Fail(ex.ToString());
            }
        }),

        Task.Run(() =>
        {
            try
            {
                while (true)
                {
                    var dc = new DataCollectionContext(new SessionId(Guid.NewGuid()));
                    string path = Path.GetTempFileName();
                    File.WriteAllText(path, "test");
                    _attachmentManager.AddAttachment(new FileTransferInformation(dc, path, true), null, new Uri("//test2"), "test2");
                    var res = _attachmentManager.GetAttachments(dc);
                }
            }
            catch (Exception ex)
            {
                Assert.Fail(ex.ToString());
            }
        })
        );
    }
  • Add try/catch on the events to avoid possible unhandled exception that lead to the test host hang.

@MarcoRossignoli MarcoRossignoli enabled auto-merge (squash) January 30, 2022 17:34
@MarcoRossignoli MarcoRossignoli marked this pull request as draft January 30, 2022 17:50
@MarcoRossignoli MarcoRossignoli marked this pull request as ready for review January 30, 2022 18:16
@MarcoRossignoli MarcoRossignoli marked this pull request as draft January 30, 2022 19:40
@MarcoRossignoli MarcoRossignoli changed the title Fix LogAttachments Fix DataCollectionAttachmentManager Jan 31, 2022
@MarcoRossignoli MarcoRossignoli changed the title Fix DataCollectionAttachmentManager Fix race condition inside DataCollectionAttachmentManager Jan 31, 2022
@MarcoRossignoli MarcoRossignoli marked this pull request as ready for review January 31, 2022 14:45
@MarcoRossignoli MarcoRossignoli enabled auto-merge (squash) January 31, 2022 14:56
@MarcoRossignoli MarcoRossignoli merged commit a209eaf into microsoft:main Jan 31, 2022
@MarcoRossignoli MarcoRossignoli deleted the fixlog branch January 31, 2022 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants