We’ve had some customers report issues with our product. Not all of them report the issue, and, although the symptoms are the same, the real issue may be different at each customer site.

While troubleshooting, I’ve stumbled upon the following code sequence (the names are meaningless, I’ve changed them as I’m not allowed to publish our source code).

public class Manager
{
/// ...
private readonly object _threadlock = new object();
private static State _fieldA;
/// ...
internal void Init()
{
    if( _fieldA != StateNotInitialized )
    {
        return;
    }
    
    lock( _threadlock )
    {
        _fieldA = RestoreFromDisk();
       /// ...
    }
}
}

I didn’t see anything wrong at the beginning. While discussing with a coworker, I’ve realized the code is prone to race condition. A more experienced coworker pointed out that the standard implementation pattern here should be the “double check locking” and also the _threadlock field should be static.
The updated code sequence is:

public class Manager
{
/// ...
/// MAKE THIS FIELD STATIC, TO REALLY LOCK ON IT!
private static readonly object _threadlock = new object();
private static State _fieldA;
/// ...
internal void Init()
{
    // Check once, to avoid the costly lock acquiring operation if it is not needed.
    if( _fieldA != StateNotInitialized )
    {
        return;
    }
    
    lock( _threadlock )
    {
        // Check again, some other thread may have already initialized the field.
        if( _fieldA != StateNotInitialized )
        {
           return;
        }
        _fieldA = RestoreFromDisk();
       /// ...
    }
}
}
Advertisements