Is this code thread and sync safe?

Question

Hi,

I need an authentication token to be thread and sync safe. The token will expire every hour and so a new one will need to be created and assigned to my static variable (TOKEN)

Will this do the trick?

Thanks,

    public static volatile string TOKEN = string.Empty;
    public static DateTime TOKEN_TIME = DateTime.Now;
    private static readonly object syncRoot = new object(); 

    public static string Get()
    {
        if (!string.IsNullOrEmpty(TOKEN))
        {
            if (!TokenIsValid())
            {
                lock(syncRoot)
                    TOKEN = CreateNewToken();
            }                
        }
        else
        {
            lock(syncRoot)
                TOKEN = CreateNewToken();
        }

        return TOKEN;
    }         

Answer 1

No, that code isn't thread-safe. Since the lock occurs inside the if statements, it is possible for two threads to create a token at roughly the same time. Consider the following:

• Thread 1 enters else block
• Thread 1 yields to Thread 2
• Thread 2 enters else block
• Thread 2 takes lock on syncRoot, creates a new token (token A), and assigns it to TOKEN
• Thread 2 returns "token A".
• Thread 2 yields to Thread 1
• Thread 1 takes lock on syncRoot, creates a new token (token B), and assigns it to TOKEN
• Thread 1 returns "token B".

Your system is now using two different tokens that were created only moments apart, and Thread references "token B".

Edit:
You can make your code threadsafe by taking a lock before you even check the token. The example below locks on every call to Get(), so it won't create two tokens at (almost) the same time like your code. There are also other locking patterns you can use, some of which may give better performance.

public static string Get()
{
    lock(syncRoot)
    {
        if (string.IsNullOrEmpty(TOKEN) || !TokenIsValid())
            TOKEN = CreateNewToken();
        return TOKEN;
    }
}