Updating property of a singleton with Thread Safety

Question

Our setup is: Asp.NET + MVC5 using AutoFac for DI.

We have a class (which is a singleton) which is managing the access tokens for a variety of services. Every now and then, these tokens get too close to expiry (less then 10 minutes) and we request new tokens, refresh them. My current implementation looks like this:

// member int used for interlocking
int m_inter = 0;

private string Token { get; set; }

private DateTimeOffset TokenExpiry { get; set; }

public SingletonClassConstructor()
{
   // Make sure the Token has some value.
   RefreshToken();
}

public string GetCredentials()
{
  if ((TokenExpiry - DateTimeOffset.UTCNow).TotalMinutes < 10)
  {
     if (Interlocked.CompareExchange(ref m_inter, 1, 0) == 0)
     {
        RefreshToken();
        m_inter = 0;
     }
  }

  return Token;
}

private void RefreshToken()
{
   // Call some stuff
   Token = X.Result().Token;
   TokenExpiry = X.Result().Expiry;
}

As you can see the Interlocked makes sure only one thread goes through and the rest gets the old Token. What I'm wondering is - can we end up in a weird situation where when the Token is being overwritten, another thread tries to read and instead of the old token, gets a partial screwed up result? Any problems with this implementation?

Thanks!

Knudsen

Why are you not just using locks? – Troudeloup

Answer 1

To me, the biggest issue with this implementation is that you may refresh the token two or more times for a single expiration period. If a thread is suspended just after checking the expiration condition but before the CompareExchange(), then another thread could get all the way through the refresh operation, including resetting m_inter, before that first thread is resumed. This can theoretically happen to arbitrarily many threads.

The remainder of your code isn't specific enough to comment on. There's no declaration of the Token type, so it's not clear whether that's a struct or class. And your GetCredentials() method is declared as returning a Credentials value, but instead returns a Token value, so that code obviously isn't even real code.

If the Token type is a class, then the rest of the implementation is probably fine. Reference type variables can be assigned atomically, even on x64 platforms, so code retrieving the Token property value will see either the old token or the new one, not some corrupted intermediate state. (I'm assuming, of course, that the Token object itself is thread-safe, preferably by virtue of being immutable.)

Personally, I would not bother with CompareExchange(). Just use a full-blown C# lock statement and be done with it. Contain the entire operation in the synchronized block: check the expiration time, replace the token if necessary, and return the token value, all from within the lock.

Based on the code you showed, I would think it would make more sense to encapsulate the whole thing in the property itself and make that public. But one way or the other, as long as code retrieving the token value can only get it through this section of synchronized code, the easiest and most reliably way to prove the code is correct is to use lock. In the unlikely event that you see performance problems with that, then you can consider alternative implementations that are harder to get right.

Ecker

Thanks a lot @Peter - this is an amazing read. To understand stuff a bit better, I've actually updated the code example, you were right, it's not the real code. The Token is of type string. I want to ask you 2 follow-up questions. 1) You mentioned about Thread being suspended. Given that this is Asp.Net would it really be suspended at that stage? No io calls etc.. can't really figure out why a thread would be suspended there (theoretically speaking you're absolutely right though 2) If I was to add the antipattern - double checking - so another if statement right before the RefreshToken? – Knudsen

"Given that this is Asp.Net would it really be suspended at that stage?" -- Windows routinely suspends all threads periodically for multitasking purposes. Even a server with an enormous number of cores (e.g. 32, 64 etc.) still has way more threads running under the OS than there are cores, and so every thread necessarily is suspended periodically. It's unavoidable. – Ecker

"If I was to add the antipattern - double checking - so another if statement right before the RefreshToken?" -- yes, you could check the expiration time again just before calling RefreshToken() and that would avoid the multiple-refresh issue. – Ecker

Looking at your updated code example, now I'm less confident about the implementation. You seem to be keeping the expiration time separately from the token. First, I don't see where this is updated, in your code example. Second, assuming it's updated in the RefreshToken() method, now you do have a clear atomicity issue. Not only can you not update Token and Minutes atomically using interlocked operation, the DateTimeOffset type is a struct and does not enjoy the atomicity guarantees of primitive types and reference type references. – Ecker

You're right in terms of Minutes being in the same RefreshToken method. Does the update need to be atomic? Can it not update the Token and Minutes in one after the other - as long as they are under the interlocked? The downside which I can think of would be the Minutes wouldn't reflect the current Token's expiry, or the current token wouldn't be the one that belongs to the minutes - for a very short time - but as long as both are updated does it need to be atomic? To be clear, I will update the code to use the locking. At this stage I'm asking because I'm learning a lot here :) – Knudsen

"Can it not update the Token and Minutes in one after the other" -- there are two problems, one very serious. First, if they are updated non-atomically, then you may still wind up updating Token multiple times (similar to before), even with double-checking, or you may get an old token (if Minutes is updated first). The latter might not be a big issue if you have plenty of buffer (e.g. the 10 minute window). The more serious problem is that you can't update DateTimeOffset atomically; it's a struct and comes with no guarantees of atomicity. – Ecker