Is the ConcurrentBag the appropriate collection?

Question

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.

You might want to turn `InMemoryRandomNumberGenerator` into a `static class`. Otherwise, you're not really describing what you want to do with with the `ConcurrentBag`. I suggest you try it, as form the information you provided I can see no reason why it would not be appropriate. — Kris Vandermotten, May 06 '14 at 22:04
@KrisVandermotten thank you for your feedback Kris. Will immediately apply the changes. — maxbeaudoin, May 06 '14 at 22:05
Surely you realised once you've tried to compile: a static class cannot implement an interface... — oleksii, May 06 '14 at 22:10
@oleksii, as a matter of fact, I have instance fields on my own class where static can't be used (this example is simplified). Good point. — maxbeaudoin, May 07 '14 at 01:07

score 1 · Accepted Answer · answered May 06 '14 at 22:08

1

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.

answered May 06 '14 at 22:08

oleksii

35,458
16
93
163

I am generating human-friendly random numbers such as 123-456-789 or ABC. I am using the Random class and set of distinct characters. I perform all the verifications that are necessary to produce a unique string. – maxbeaudoin May 07 '14 at 01:12
@maxbeaudoin I remember to have written a random string extension, maybe you'd [have a look](http://stackoverflow.com/a/8683325/706456). There are a few other alternative answers there as well. This is in case you need a random string. – oleksii May 07 '14 at 09:43
I have used a very similar approach inspired by [this post](http://stackoverflow.com/questions/1344221/how-can-i-generate-random-alphanumeric-strings-in-c). Can you take a look at my answer below? – maxbeaudoin May 07 '14 at 14:43

score 0 · Answer 2 · answered May 07 '14 at 04:04

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

Is the ConcurrentBag the appropriate collection?

2 Answers2