Luke Vo Luke Vo -3 years ago 91
C# Question

Is locking between two same conditional statements good pattern?

I am writing a web application, so concurrency is concerned. I need to call an API with a token that is expired every 10 minutes, so before every API call, I need to check for new tokens.

I do not have any experience with concurrent design, so this is what I am doing and I need someone to clarify if I am doing well. Is it anti-pattern? Note: the class itself is singleton.

private object tokenLock = new object();
private void RequestApi ()
{
if (DateTime.UtcNow >= this.TokenExpireTime)
{
lock (this.tokenLock)
{
if (DateTime.UtcNow >= this.TokenExpireTime)
{
// Request new Token
// Update new Token Expiration Time
}
}
}

// Request API
}


To prevent duplicated code, I can rewrite into a property (logic is still the same):

private void RequestApi ()
{
if (this.NeedNewToken)
{
lock (this.tokenLock)
{
if (this.NeedNewToken)
{
// Request new Token
// Update new Token Expiration Time
}
}
}

// Request API
}

private bool NeedNewToken => DateTime.UtcNow >= this.TokenExpireTime;

Answer Source

You should get rid of the outer, unsynchronized comparison.

First of all, the pattern you're using is called "double-checked locking". It's generally used only in situations where there's a high level of contention and a low likelihood of needing to take the lock, justifying the awkwardness of the code.

On that basis alone, it's unlikely you really need to use it in this case. Though, I admit that in that regard, the question is a matter of opinion.

But more important, the double-checked locking depends on you being able to do the conditional test itself safely without synchronization, which is not the case here. The DateTime value is not a primitive, and so can't be safely accessed without synchronization. Otherwise, you run the risk of getting a "torn" value, i.e. a value that is partially written and so is neither the old value nor the new value.

In this respect, the question is not a matter of opinion. The code is simply incorrect and needs to be fixed. Just take the lock, and do whatever you need to do inside the lock. Don't try to short-cut with an unsynchronized comparison. It's not safe in this case.

Recommended from our users: Dynamic Network Monitoring from WhatsUp Gold from IPSwitch. Free Download