which part of this is not thread safe?

[Travis Laborde]

In the below code, I'm looking for one document in a collection by KeyName.  If it does not exist, I'm inserting it with a KeyValue of 0.  then I'm adjusting it "up" by one by calling the AdjustCounterAndReturnAdjustedValue method.  In that method I'm finding the document.  It is known to exist because if it didn't I would have just created it.  So I'm incrementing the KeyValue by 1.  if I put this in a loop and run it any number of times, it works fine.  For example, if I call this 1001 times, the last returned value is 1001.  But when I do this with multiple threads each running the same code...  it doesn't work as expected.  I spin up 4 threads, having each do this1000 times.  Once those threads are done I do it one more time, expecting the result to be 4001, but it is always 1001.  This is with the C# driver.  Any ideas?

         public int IncrementCounter(string keyName)

         {

            // snip some code here that gets the collection...

            var existing = collection.AsQueryable().SingleOrDefault(foo => foo.KeyName == keyName);

            if (existing == null)

            {

                collection.Insert(new CounterDocument() {KeyName = keyName, KeyValue = 0});

            }

            var val = AdjustCounterAndReturnAdjustedValue(collection, keyName, 1);

            return val;

          }

        private int AdjustCounterAndReturnAdjustedValue(MongoCollection collection, string keyName, int adjustmentAmount)

        {

            var query = Query.And(Query.EQ("_id", keyName));

            var sortBy = SortBy.Null;

            var update = Update.Inc("KeyValue", adjustmentAmount);

            var result = collection.FindAndModify(query, sortBy, update, true);

            var val = result.GetModifiedDocumentAs<CounterDocument>().KeyValue;

            return val;

        }

[craiggwilson]

Can you show us your entire program so that we can try and reproduce on our side?

[Travis Laborde]

OK, I've boiled it down to something that I can just paste here.  Obviously, you'd need the Mongo driver dlls and NUnit to run this.  The SingleThreadTest passes every time.  The MultiThreadedTest fails every time, but sometimes with different results :)

using System.Linq;

using System.Threading;

using MongoDB.Bson.Serialization.Attributes;

using MongoDB.Driver;

using MongoDB.Driver.Builders;

using MongoDB.Driver.Linq;

using NUnit.Framework;

namespace ThreadFail

{

    public class CounterDocument

    {

        [BsonId]

        public string KeyName { get; set; }

        public int KeyValue { get; set; }

    }

    [TestFixture]

    public class Tests

    {

        private MongoServer _server;

        private MongoDatabase _database;

        private const string testCollectionName = "CounterDocuments";

        private const string testCounterName = "mycounter";

        [SetUp]

        public void SetUp()

        {

            // NUnit runs this before each test

            _server = MongoServer.Create("mongodb://localhost:9123"); 

            _database = _server.GetDatabase("threadfail", SafeMode.True);

            _database.DropCollection(testCollectionName);

        }

        private int IncrementCounter(string keyName)

        {

            var collection = _database.GetCollection<CounterDocument>(testCollectionName);

            var existing = collection.AsQueryable().SingleOrDefault(foo => foo.KeyName == keyName);

            if (existing == null)

            {

                collection.Insert(new CounterDocument() { KeyName = keyName, KeyValue = 0 });

            }

            var val = AdjustCounterAndReturnAdjustedValue(collection, keyName, 1);

            return val;

        }

        private int AdjustCounterAndReturnAdjustedValue(MongoCollection collection, string keyName, int adjustmentAmount)

        {

            var query = Query.And(Query.EQ("_id", keyName));

            var sortBy = SortBy.Null;

            var update = Update.Inc("KeyValue", adjustmentAmount);

            var result = collection.FindAndModify(query, sortBy, update, true);

            var val = result.GetModifiedDocumentAs<CounterDocument>().KeyValue;

            return val;

        }

        [Test]

        public void SingleThreadedTest()

        {

            IncrementTheCounter1000Times();

            var valueNow = IncrementCounter(testCounterName);

            Assert.That(valueNow == 1001, "should have been 1001 but was {0}", valueNow);

        }

        [Test]

        public void MultiThreadedTest()

        {

            var thread1 = new Thread(IncrementTheCounter1000Times);

            var thread2 = new Thread(IncrementTheCounter1000Times);

            var thread3 = new Thread(IncrementTheCounter1000Times);

            var thread4 = new Thread(IncrementTheCounter1000Times);

            thread1.Start();

            thread2.Start();

            thread3.Start();

            thread4.Start();

            while (thread1.IsAlive || thread2.IsAlive || thread3.IsAlive || thread4.IsAlive)

            {

                // keep the unit test alive

            }

            var valueNow = IncrementCounter(testCounterName);

            Assert.That(valueNow == 4001, "should be up to 4001 now, but was {0}", valueNow);

        }

        private void IncrementTheCounter1000Times()

        {

            for (int i = 0; i < 1000; i++)

            {

                IncrementCounter(testCounterName);

            }

        }

    }

}

[craiggwilson]

Hi Travis,

  Sorry for the late reply.  So, I ran your code as a console program instead of through nunit and it worked just as it should have.  I ended up with 1001 and 4001 respectively.  Perhaps there is something about running this with NUnit that causes things to behave differently.  Would you mind trying this as a console program and letting me know the results?

[craiggwilson]

Ok, I've got this running in NUnit just fine as well.  I did have to add an IncrementCounter(testCounterName) at the top of Multithreaded test, making the result 4002 instead of 4001.  This was because the first write to the document would create it and more than one thread ended up in trying to create the document at the same time.  Therefore, I forced creation at the top and then everything else ran fine.

What server version are you using?  What driver version are you running?

[Travis Laborde]

Craig - thanks.  But isn't that the point?  Where you say that you had to add one call before spinning up the threads, or else more than one thread would end up trying to create the document at the same time...

That was the point of the test.  There is only one Mongo Server running, yet there are multiple threads calling in.  If thread #1 causes a new document to be created, thread #2 should know that and do an update instead, right?  The code is trying to either insert or increment.  Perhaps it's just a race condition instead?  That thread #2 sees "null" and tries to do an insert then fails because by then thread #1 has already done an insert perhaps?

Travis

[Scott Hernandez]

It is a race condition/poorly-concurrent code. The time between the
query+insert is non-zero and doesn't happen atomically.

Remove your query+insert and just always do the findAndModify or an
update and it will work just fine.

Here is a possible sequence which would cause unexpected results:

Thread1: query
Thread2: query
Thread3: query
Thread3: insert //doc
Thread1: insert //error
Thread3: findAndModify //val 1
Thread2: insert //error
Thread4: query
Thread4: findAndModify //val 2

value = 2 // not what you want.

> --
> You received this message because you are subscribed to the Google
> Groups "mongodb-user" group.
> To post to this group, send email to mongod...@googlegroups.com
> To unsubscribe from this group, send email to
> mongodb-user...@googlegroups.com
> See also the IRC channel -- freenode.net#mongodb

[Scott Hernandez]

I noticed one other issue with your code, you need to use the upsert
flag to indicate that if the document can't be found then it will be
inserted.

Maybe you meant to include that, or maybe that was the conceptual key
you were missing.

You can read more about the upsert option here :
http://www.mongodb.org/display/DOCS/findAndModify+Command
http://www.mongodb.org/display/DOCS/Updating