How to make a threadsafe call to a 3rd party API (OAuth)?

Question

I'm calling a 3rd party API that uses OAuth for authentication, and I'm wondering how to make this threadsafe:

var token = _tokenService.GetCurrentToken(); // eg token could be "ABCDEF"
var newToken = oauth.RenewAccessToken(token); // eg newToken could be "123456"
_tokenService.UpdateCurrentToken(newToken); // save newToken to the database

What this does is to use the previous token every time RenewAccessToken() is called. But there is a problem if two users initiate this at the same time (two different threads will run the code at the same time), and we end up with that code executed in this order:

[Thread 1] var token = _tokenService.GetCurrentToken(); // returns "ABCDEF"
[Thread 2] var token = _tokenService.GetCurrentToken(); // returns "ABCDEF"
[Thread 1] var newToken = oauth.RenewAccessToken("ABCDEF"); // returns "123456"
[Thread 2] var newToken = oauth.RenewAccessToken("ABCDEF"); 
           // throws an invalid token exception

What has happened is that in thread 2, it should actually be calling oauth.RenewAccessToken("123456"); (because that is the latest token value. But the latest token hasnt even been saved to the database yet, so thread 2 always has the wrong value for current token.

What can I do to fix this?

Edit: It has been suggested to use a lock like this:

private object tokenLock = new object();
lock(tokenLock)
{
    var token = _tokenService.GetCurrentToken(); 
    var newToken = oauth.RenewAccessToken(token); 
    _tokenService.UpdateCurrentToken(newToken);
}

Edit 2: The lock didn't actually work anyway, this is from my logs:

[43 22:38:26:9963] Renewing now using token JHCBTW1ZI96FF 
[36 22:38:26:9963] Renewing now using token JHCBTW1ZI96FF 
[36 22:38:29:1790] OAuthException exception

The first number is the thread id and the second is a timestamp. Both threads executed at the exact same time down to the milliseconds. I don't know why the lock failed to stop thread 36 until after thread 43 had finished.

Edit 3: And again, this time after changing the object tokenLock to be a class variable instead of a local variable, the lock did not work.

[25 10:53:58:3870] Renewing now using token N95984XVORY
[9 10:53:58:3948] Renewing now using token N95984XVORY
[9 10:54:55:7981] OAuthException exception

Adam Robinson · Accepted Answer · 2011-06-02T14:43:53.320

4

EDIT

Given that this is an ASP.NET application, the easy route (a Monitor lock using a lock { } block) is not suitable. You'll need to use a named Mutex in order to solve this problem.

Given your example code, something along these lines would work:

using(var m = new Mutex("OAuthToken"))
{
    m.WaitOne();

    try
    {
        var token = _tokenService.GetCurrentToken(); 
        var newToken = oauth.RenewAccessToken(token); 
        _tokenService.UpdateCurrentToken(newToken);
    }
    finally
    {
        m.ReleaseMutex();
    }
}

Note the finally clause; it's very important that you release the mutex. Because it's a system-wide object, its state will persist beyond your application. If you were to encounter an exception in your OAuth code above, you would not be able to reenter the code until the system was restarted.

Also, if you have some sort of durable identifier for sessions that use the same OAuth token (something that won't get changed as a result of this process), you could potentially use that token as the mutex name instead of "OAuth" as I have above. This would make the synchronization specific to a given token, so you would not have to worry about operations having to wait on unrelated tokens being renewed. This should offset the increase in cost of a mutex over a Monitor lock.

For the sake of helping others who might find this question, I've left my original answer below:

Original Answer

You just need a simple lock around your operation.

Create an instance (or static, if these functions are static) variable of type object:

private object tokenLock = new object();

In your code, enclose the steps that need to be atomic within a lock(tokenLock) block:

lock(tokenLock)
{
    var token = _tokenService.GetCurrentToken(); 
    var newToken = oauth.RenewAccessToken(token); 
    _tokenService.UpdateCurrentToken(newToken);
}

This will prevent one thread from starting this process while another is executing it.

edited Jun 02 '11 at 14:43

answered Jun 01 '11 at 02:02

Adam Robinson

182,639
35
285
343

@Adam - I've seen a potential problem (see question edit). Those 3 lines are used by every user of the system (frequently). So if there is a lock around it, could it cause a serious bottleneck? – JK. Jun 01 '11 at 07:51
@JK: Do not lock on value types (like `Guid`). You will get a different lock object every time, as the value type will be boxed into a brand new reference each time. Go with the simple lock and don't worry about it being a bottleneck unless it shows itself to be so; an alternative will be significantly more complex. – Adam Robinson Jun 01 '11 at 11:15
@JK: Additionally, the type of application you're developing will determine how this needs to be done. If this is an ASP.NET application or WCF service, then this likely won't work as each user will get a new lock object every time the service is invoked. For this, you'll probably have to use a System mutex so that the state is persistent across invocations. If this is the case, let me know and I'll modify my answer later this morning. – Adam Robinson Jun 01 '11 at 11:17
@Adam yes I forgot to say, it is an ASP.NET MVC2 app. – JK. Jun 01 '11 at 12:09
@Adam - I had the `object tokenLock` as a local variable instead of a class variable. I've updated it just now, will see if that improves the situation. – JK. Jun 01 '11 at 22:42
@Adam - no that still doesn't lock. I've been reading the various lock vs mutex questions eg http://stackoverflow.com/questions/1164038/monitor-vs-mutex-in-c but I don't see anything that says that lock() does not work for ASP.NET apps. – JK. Jun 01 '11 at 23:01
@JK: It's not that locks don't work for ASP.NET applications, but the fact that distinct ASP.NET object instances are not shared among concurrent calls means that ordinary `Monitor` locks are not useful for preventing interference between concurrent calls. You need to use a named mutex (as that answer suggests), as what you're doing is essentially a cross-process lock (it may not, in fact, be another process, but you have to treat it as if it were). – Adam Robinson Jun 02 '11 at 14:35
@JK: I have edited my answer and added a mutex example. Let me know if this helps. – Adam Robinson Jun 02 '11 at 14:44

How to make a threadsafe call to a 3rd party API (OAuth)?

1 Answers1