GetOrAdd Ran My Factory Three Times

I had a client object that was expensive to build. It opens a pooled connection, reads a bit of config, and I only ever want one per name for the life of the process. So I did the obvious thing and cached it in a ConcurrentDictionary, keyed by name, built on first use with GetOrAdd. Thread-safe dictionary, lazy construction, done.

Then I noticed the startup logs. For a single key, "opening connection" showed up three times. Only one connection was actually in the dictionary afterward, and every caller was using it, so nothing looked broken downstream. But two connections had been opened and then quietly abandoned. On a cold start with a few of these firing at once, that adds up to real work nobody asked for.

The race is hiding in the one line I trusted most.

Where the extra calls come from

GetOrAdd(key, valueFactory) is not a lock around your factory. When several threads hit a missing key at the same moment, more than one of them can run the factory. The dictionary only keeps one of the results and hands that same instance to everyone, but the losing factories already executed. They opened their connection, did their work, and got thrown away.

I didn't want to take that on faith, so I built the smallest thing that shows it. Sixty-four threads, one key, a factory that stamps a serial number onto each object it builds and spins a little to mimic real setup cost:

static int _factoryCalls;

static Connection BuildConnection()
{
    int serial = Interlocked.Increment(ref _factoryCalls);
    Thread.SpinWait(200_000); // stand in for a handshake / DNS / file read
    return new Connection(serial);
}

Parallel.For(0, 64, _ =>
{
    var conn = cache.GetOrAdd("db", _ => BuildConnection());
    handedOut.Add(conn.SerialNumber);
});

Here's what that run printed:

== GetOrAdd(key, factory) ==
  factory ran:            3 time(s)
  connections built:      3 (and 2 thrown away)
  serial stored in cache: #2
  distinct serials callers actually got: 1

Three factory calls. All 64 callers walked away with the same connection, so the "distinct serials" count is one, which is exactly why this is so easy to miss. The cache is doing its job. It just paid three times to fill one slot. Notice too that the instance it kept was serial #2, not #1. The winner is whichever thread finishes the insert first, and that's not the one that started first.

That SpinWait widens the window so the race is obvious, but it doesn't create it. Take the spin out and you still get duplicate calls now and then, you just need more threads or worse luck to see them. The count isn't stable either. I got three every time on this little container because it doesn't have many cores. On a 16-core box under a real stampede I've seen it climb a lot higher.

One Lazy and it stops

The fix I reach for is to stop storing the connection and start storing a Lazy<T> of it. Building a Lazy is cheap, so it doesn't matter if GetOrAdd makes a few of those. Only the winner's .Value ever gets touched, and Lazy guarantees the real factory behind it runs exactly once:

var cache = new ConcurrentDictionary<string, Lazy<Connection>>();

var conn = cache.GetOrAdd("db",
    _ => new Lazy<Connection>(BuildConnection, LazyThreadSafetyMode.ExecutionAndPublication)).Value;

Same 64 threads, same key:

== GetOrAdd(key, new Lazy<T>(factory)).Value ==
  factory ran:            1 time(s)
  distinct serials callers actually got: 1

One call. That's the whole trick. The dictionary is still allowed to race on the wrapper, but the expensive part is behind a gate that only opens once. ExecutionAndPublication is the default mode and it's the one you want here: it both runs the factory once and makes sure everyone sees the same finished object.

When I don't bother

I don't wrap everything in Lazy out of reflex, and I don't think you should either. If the factory is cheap and has no side effects, building a throwaway int or a small record twice costs you nothing you'll ever measure, and the wrapper just adds an allocation and a layer of .Value noise to read past. The plain GetOrAdd is fine there, and honestly it's the more readable code.

The Lazy version earns its keep when the factory is expensive, or when running it twice is a bug and not just waste: it opens a socket, writes a file, increments something, registers a handler. That's the real tell. Not "is this slow" but "what happens if this runs more than once." If the answer is "nothing good," reach for the wrapper.

One more thing worth saying out loud: this was never a thread-safety hole. Nobody got a half-built object and the dictionary never got corrupted. It's a "ran more than I expected" problem, which is a quieter kind of bug and the sort that sits in your startup path for months looking like a slightly slow boot.

Here's the whole thing in one file so you can run it yourself. It's a plain net10.0 console app, nothing else in the project:

using System.Collections.Concurrent;
using System.Diagnostics;

sealed class Connection
{
    public int SerialNumber { get; }
    public Connection(int serial) => SerialNumber = serial;
}

static class Program
{
    static int _factoryCalls;

    static Connection BuildConnection()
    {
        int serial = Interlocked.Increment(ref _factoryCalls);
        Thread.SpinWait(200_000); // stand in for a handshake / DNS / file read
        return new Connection(serial);
    }

    static void Main()
    {
        const int callers = 64;
        Console.WriteLine($"Spinning up {callers} threads, all asking for the same key at once.\n");
        RunPlain(callers);
        Console.WriteLine();
        RunLazy(callers);
    }

    static void RunPlain(int callers)
    {
        _factoryCalls = 0;
        var cache = new ConcurrentDictionary<string, Connection>();
        var handedOut = new ConcurrentBag<int>();

        var sw = Stopwatch.StartNew();
        Parallel.For(0, callers, _ =>
        {
            var conn = cache.GetOrAdd("db", _ => BuildConnection());
            handedOut.Add(conn.SerialNumber);
        });
        sw.Stop();

        Console.WriteLine("== GetOrAdd(key, factory) ==");
        Console.WriteLine($"  factory ran:            {_factoryCalls} time(s)");
        Console.WriteLine($"  connections built:      {_factoryCalls} (and {_factoryCalls - 1} thrown away)");
        Console.WriteLine($"  serial stored in cache: #{cache["db"].SerialNumber}");
        Console.WriteLine($"  distinct serials callers got: {handedOut.Distinct().Count()}");
        Console.WriteLine($"  elapsed:                {sw.Elapsed.TotalMilliseconds:F1} ms");
    }

    static void RunLazy(int callers)
    {
        _factoryCalls = 0;
        var cache = new ConcurrentDictionary<string, Lazy<Connection>>();
        var handedOut = new ConcurrentBag<int>();

        var sw = Stopwatch.StartNew();
        Parallel.For(0, callers, _ =>
        {
            var conn = cache.GetOrAdd("db",
                _ => new Lazy<Connection>(BuildConnection, LazyThreadSafetyMode.ExecutionAndPublication)).Value;
            handedOut.Add(conn.SerialNumber);
        });
        sw.Stop();

        Console.WriteLine("== GetOrAdd(key, new Lazy<T>(factory)).Value ==");
        Console.WriteLine($"  factory ran:            {_factoryCalls} time(s)");
        Console.WriteLine($"  distinct serials callers got: {handedOut.Distinct().Count()}");
        Console.WriteLine($"  elapsed:                {sw.Elapsed.TotalMilliseconds:F1} ms");
    }
}

Have you been bitten by this one? I'm curious whether people hit it through an expensive factory like I did or through a factory with a side effect that broke something louder.

— still counting things that were only ever supposed to run once.