Library for doing A/B tests in a web app

I made a simple library to help me doing A/B tests in a web app. The idea is simple: I have two or more page options (URL) for a given page and every call to the library method should give me a URL so at the end all options were given the same traffic.

My questions are:

• Is this 100% thread safe?

• Is this performatic (I'm worried about the locks in a multithreaded (web) environment)?

• Did I use the best data structure for it?

• Can it be more readable?

Here are the tests and the implementation:

```
[TestFixture]
public class ABTestTest
{
[SetUp]
public void Setup()
{
ABTest.ResetAll();
}

[Test]
public void GetUrlWithTwoCandidates()
{
ABTest.RegisterUrl("CarrinhoPagamento", "Url1.aspx");
ABTest.RegisterUrl("CarrinhoPagamento", "Url2.aspx");

ABTest.GetUrl("CarrinhoPagamento").Should().Be.EqualTo("Url1.aspx");
ABTest.GetUrl("CarrinhoPagamento").Should().Be.EqualTo("Url2.aspx");
ABTest.GetUrl("CarrinhoPagamento").Should().Be.EqualTo("Url1.aspx");
ABTest.GetUrl("CarrinhoPagamento").Should().Be.EqualTo("Url2.aspx");
ABTest.GetUrl("CarrinhoPagamento").Should().Be.EqualTo("Url1.aspx");
ABTest.GetUrl("CarrinhoPagamento").Should().Be.EqualTo("Url2.aspx");
ABTest.GetUrl("CarrinhoPagamento").Should().Be.EqualTo("Url1.aspx");
}

[Test]
public void GetUrlWithThreeCandidates()
{
var OptionsSelected = new Dictionary();
OptionsSelected.Add("Url1.aspx", 0);
OptionsSelected.Add("Url2.aspx", 0);
OptionsSelected.Add("Url3.aspx", 0);

ABTest.RegisterUrl("CarrinhoPagamento", "Url1.aspx");
ABTest.RegisterUrl("CarrinhoPagamento", "Url2.aspx");
ABTest.RegisterUrl("CarrinhoPagamento", "Url3.aspx");

var nextUrl = "";
for (int i = 1; i ();
OptionsSelected.Add("Url1.aspx", 0);
OptionsSelected.Add("Url2.aspx", 0);
OptionsSelected.Add("Url3.aspx", 0);

A

This part is not thread safe:

lock (NextOption)
    {
        NextOption = new Hashtable();
    }

The problem is that the object being used to synchronize is re-assigned. That means a subsequent caller will acquire a different lock.

It's conceivable that two writer threads hit RegisterUrl and each see a different lock, but end up adding concurrently to the same hash table, which is a harmful race condition. There are also more subtle problems if two threads are both calling ResetAll and a third thread is inserting.

You should do something like this:

object NextOptionLock = new object();
Hashtable NextOption = new Hashtable();

lock (NextOptionLock)
{
    NextOption = new HashTable();
}

I would do this for all other places where you use lock(/ ... /). It's a good practice to keep the lock object separate from the data being protected.

This next part looked kind of suspicious to me because its thread safety depends on the implementation of Hashtable:

if (Options.ContainsKey(key.GetHashCode()))
    {
        lock (Options)
        {
            ((List)Options[key.GetHashCode()]).Add(new ABTestOption()

The question is whether or not it's safe to call ContainsKey while another thread does Add. According to Microsoft's documentation it is safe:

Hashtable is thread safe for use by multiple reader threads and a single writing thread. It is thread safe for multi-thread use when only one of the threads perform write (update) operations, which allows for lock-free reads provided that the writers are serialized to the Hashtable.

So while it might be a red flag for someone auditing for concurrency problems, it seems OK.

Your use of GetHashCode looks a little weird. Don't MS's classes do this for you when you use an object as a hash table key?

Also, why use a non-generic class like Hashtable? I see from MSDN that Dictionary does not allow reads to be overlapped with a write in the same way that Hashtable's documentation claims, so maybe that's the reason... Have you looked at ConcurrentDictionary?

StackExchange Code Review Q#16211, answer score: 2