Race condition in dispose

[matt-richardson]

We're facing a (rare) race condition in Rnwood.SmtpServer.SmtpServer.KillConnections:

System.InvalidOperationException : Collection was modified; enumeration operation may not execute.
   at System.Collections.Generic.List`1.Enumerator.MoveNextRare()
at System.Linq.Enumerable.CastIterator[TResult](IEnumerable source)+MoveNext()
at System.Collections.Generic.LargeArrayBuilder`1.AddRange(IEnumerable`1 items)
at System.Collections.Generic.EnumerableHelpers.ToArray[T](IEnumerable`1 source)
at Rnwood.SmtpServer.SmtpServer.KillConnections()
at Rnwood.SmtpServer.SmtpServer.Stop(Boolean killConnections)
at Rnwood.SmtpServer.SmtpServer.Stop()
at Rnwood.SmtpServer.SmtpServer.Dispose(Boolean disposing)
at Rnwood.SmtpServer.SmtpServer.Dispose()
at Octopus.IntegrationTests.Core.EmailSending.SmtpClientWrapperFixture.WhenSendingAnEmailToAServerWithNoTls_TheEmailShouldBeAccepted() in /opt/buildagent/work/abb2fbfce959a439/source/Octopus.IntegrationTests/Core/EmailSending/SmtpClientWrapperFixture.cs:line 44
at NUnit.Framework.Internal.TaskAwaitAdapter.GenericAdapter`1.BlockUntilCompleted()
at NUnit.Framework.Internal.MessagePumpStrategy.NoMessagePumpStrategy.WaitForCompletion(AwaitAdapter awaitable)
at NUnit.Framework.Internal.AsyncToSyncAdapter.Await(Func`1 invoke)
at NUnit.Framework.Internal.Commands.TestMethodCommand.RunTestMethod(TestExecutionContext context)
at NUnit.Framework.Internal.Commands.TestMethodCommand.Execute(TestExecutionContext context)
at NUnit.Framework.Internal.Execution.SimpleWorkItem.PerformWork()
------- Stdout: -------
11/30/2020 9:33:24 PM 34 Trace Connected to SMTP server smtp://localhost:37175/?starttls=when-available
11/30/2020 9:33:24 PM 34 Trace SMTP received: 220 ip-10-10-11-232 smtp4dev ready
11/30/2020 9:33:24 PM 34 Trace SMTP sent: EHLO [127.0.0.1]
11/30/2020 9:33:24 PM 34 Trace SMTP received: 250-Nice to meet you.
250-8BITMIME
250-SIZE
250 SMTPUTF8
11/30/2020 9:33:24 PM 28 Trace SMTP sent: MAIL FROM:<sender@example.com>
11/30/2020 9:33:24 PM 28 Trace SMTP received: 250 New message started
11/30/2020 9:33:24 PM 32 Trace SMTP sent: RCPT TO:<recipient@example.com>
11/30/2020 9:33:24 PM 31 Trace SMTP received: 250 Recipient accepted
11/30/2020 9:33:24 PM 32 Trace SMTP sent: DATA
11/30/2020 9:33:24 PM 32 Trace SMTP received: 354 End message with period
11/30/2020 9:33:24 PM 32 Trace SMTP sent: From: sender@example.com
To: recipient@example.com
Subject: Test subject
Date: Mon, 30 Nov 2020 21:33:24 +1000
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
11/30/2020 9:33:24 PM 32 Trace SMTP sent:
Test body
11/30/2020 9:33:24 PM 28 Trace SMTP sent:
.
11/30/2020 9:33:24 PM 12 Trace SMTP received: 250 Mail accepted
11/30/2020 9:33:24 PM 34 Trace SMTP sent: QUIT
11/30/2020 9:33:24 PM 12 Trace SMTP received: 221 Goodbye

This is pretty rare:


(first one is testing async, second is testing sync)

Let me know if you want to see the tests/code in question - I dont think it's too important here.

Looking into the code, i think this line is causing the error. Even though activeConnections is a synchronized list, it's not threadsafe for enumeration. Looking at the docs, it seems the code needs to lock on the SyncRoot property before enumeration:

lock(myCollection.SyncRoot)
{
    foreach (object item in myCollection)
    {
        // Insert your code here.
    }
}

Would you be willing to accept a PR that changes this?

[rnwood]

Hi Matt.

Thanks for the great and detailed bug report (and excited to see this component being used in your work!).

I agree that the line you've found is incorrect WRT thread safety and will throw if the collection is modified by one of the connections being closed in another thread whilst the .ToArray() is being executed.

Yes please for the PR to fix this.

Thanks
Rob