Thread safe factory

Question

I have a factory which job is to create new instances on each call when such are needed. My code looks like that:

public class Factory
{
    public object Get()
        => new object();
}

I try testing the multi threading part with such nunit test:

public async Task Get_ThreadSaftyTests()
{
    const int limit = 10_000
    var concurrentCollection = new ConcurrentBag<object>();
    var instance = new Factory();
    var tasks = new List<Task>(limit);

    for (int i = 0; i < limit; i++)
    {
        tasks.Add(Task.Factory.StartNew(() =>
        {
            concurrentCollection.Add(instance.Get());
        }));
    }

    await Task.WhenAll(tasks);

    var hashSet = new HashSet<long>();
    foreach (var item in concurrentCollection)
    {
        hashSet.Add(item.GetHashCode());
    }

    Assert.AreEqual(limit, hashSet.Count);
}

This test passes most of the time. 4/5 to be exact. Which is a bit strange.

I have tried to implement a lock on before returning the instance like so:

public class Factory
{
    private static readonly Lazy<object> lockObject = new Lazy<object>(true);
    
    public object Get()
    {
        lock (lockObject.Value)
        {
            return new object();
        }
    }
}

but with that implementation the test almost never passes. 1/5 runs the test passes. The question here is what am i doing wrong?

------------------------------update-------------------------------

If the loop has 100 000 iterations the test fails every time in both cases.

If the loop has 6 500 iterations it passes every time in both cases.

That's a lot of tasks, especially when using `StartNew`, it's pretty unusual to need to create so many. Normally you would create only as many as you have individual processors — Charlieface, May 31 '21 at 10:36
The test is completely pointless. It does nothing useful. The main issue is that you are getting 100,000 objects and then seeing if there are any duplicate hash codes. Which is - well it is pointless. There is no guarantee the hash code for each of the 100,000 objects will be unique. This is nothing to do with thread safety though - it is just a very weird thing to test. I suspect what you _should_ test is not a `HashSet` but a `HashSet — mjwills, May 31 '21 at 11:17
As a side note, `Task.Run` [is preferable](https://devblogs.microsoft.com/pfxteam/task-run-vs-task-factory-startnew/) to `Task.Factory.StartNew` in general. Also the `ConcurrentBag` is an [extremely specialized](https://stackoverflow.com/a/64823123/11178549) collection, with very few practical uses, and your example is not one of them. A `ConcurrentQueue` is preferable because it preserves the order of the inserted items, but even better would be to not use a concurrent collection at all, have a `List>` instead of `List`, and `var objects = await Task.WhenAll(tasks)`. — Theodor Zoulias, May 31 '21 at 13:06

JonasH · Accepted Answer · 2021-05-31T11:26:52.187

    lock (lockObject.Value)
    {
        return new object();
    }

The lock does nothing in this case. Calling a constructor may or may not be threadsafe, but the there is nothing in the framework that makes creating objects non-threadsafe. I.e. you would need to write a non-threadsafe constructor yourself, and then it is up to you to use it correctly.

However:

var hashSet = new HashSet<long>();
foreach (var item in concurrentCollection)
{
    hashSet.Add(item.GetHashCode());
}

GetHashCode is not guaranteed to return an unique number, it cannot since references are probably 64-bit, while GetHashCode returns a 32-bit number.

The fix should be simple. Change to HashSet<object> and remove .GetHashCode()

Thread safe factory

1 Answers1