CODEWITHSUNDEEP
c#randomconcurrencyasp.net-web-apithread-safety
maxbeaudoin
maxbeaudoin

Reputation: 6976

Is the ConcurrentBag<T> the appropriate collection?

I'm in a situation where I have a ASP.NET Web API 2 project hosted on IIS and I'm anticipating concurrency issues. I need to generate random numbers and make sure that they are unique (later stored in the database). To do so, I have implemented a simple in-memory RNG that rely on a static ConcurrentBag. I am aware that this implementation would imply risks on a distributed architecture. Rapidly, the code looks like this :

public interface IRandomNumberGenerator
{
    string ReserveNumber();
    string ReleaseNumber(string number);
}

public class InMemoryRandomNumberGenerator : IRandomNumberGenerator
{
    private static readonly ConcurrentBag<string> Bag = new ConcurrentBag<string>();

    public string ReserveNumber()
    {
        // Add
        throw new NotImplementedException();
    }

    public string ReleaseNumber(string number)
    {
        // Remove
        throw new NotImplementedException();
    }
}

This code is intended to be used like so :

var number = rng.ReserveNumber();

StoreIntoDatabase(number);

rng.ReleaseNumber(number);

Am I using the ConcurrentBag collection appropriately?

Also note that I have simplified my example and that I am not interested in moving code into SQL and use SQL transaction to accomplish this task.

Upvotes: 0

Views: 702

Answers (2)

maxbeaudoin
maxbeaudoin

Reputation: 6976

I have revised the design. I switched to ConcurrentDictionary to avoid duplicates as pointed out by @oleksii. I use a byte because I don't use the value and there is no ConcurrentHashset to my knowledge.

NUnit test :

[Test]
public void GenerateStrings()
{
    var gen1 = new ConcurrentStringGenerator("0123456789", 9);

    for (int i = 0; i < 100; i++)
    {
        var str = gen1.Reserve();
        Console.WriteLine(int.Parse(str).ToString("000-000-000"));
        Assert.True(gen1.Release(str));
    }

    var gen2 = new ConcurrentStringGenerator("ABCDEFGHJKLMNPQRSTUVWXYZ", 3);

    for (int i = 0; i < 100; i++)
    {
        var str = gen2.Reserve();
        Console.WriteLine(str);
        Assert.True(gen2.Release(str));
    }
}

Implementation :

public class ConcurrentStringGenerator
{
    private readonly Random _random;
    private readonly string _charset;
    private readonly int _length;
    private readonly ConcurrentDictionary<string, byte> _numbers;

    public ConcurrentStringGenerator(string charset, int length)
    {
        _charset = charset;
        _length = length;
        _random = new Random();
        _numbers = new ConcurrentDictionary<string, byte>();
    }

    public string Reserve()
    {
        var str = Generate();
        while (!_numbers.TryAdd(str, 0))
        {
            str = Generate();
        }
        return str;
    }

    public bool Release(string str)
    {
        byte b;
        return _numbers.TryRemove(str, out b);
    }

    private string Generate()
    {
        return new string(Enumerable.Repeat(_charset, _length).Select(s => s[_random.Next(s.Length)]).ToArray());
    }
}

@oleksii as for the protected section, I'm trying to avoid a lock statement over the sequence and use a concurrent collection instead. Can you be more specific on the following statement?

You need to have some sort of a protected section or a transaction for this sequence, otherwise concurrency issue may appear

Upvotes: 0

oleksii
oleksii

Reputation: 35905

I guess you trying to solve a concurrency problem where many users click a button to generate a number. While ConcurrentBag might be OK to use from the concurrency perspective I see other problems:

  • "Bags are useful for storing objects when ordering doesn't matter, and unlike sets, bags support duplicates. " msdn. I think you were trying to avoid duplicates.

  • You need to have some sort of a protected section or a transaction for this sequence, otherwise concurrency issue may appear

    var number = rng.ReserveNumber();
StoreIntoDatabase(number);
rng.ReleaseNumber(number);

I hope you don't roll out your own RNG, but reuse something like RNGCryptoServiceProvider.

Upvotes: 1

Related Questions