Thread safe accessing memory cache using semaphore

Question

I have a Web API and for some requests we are sending requests to a third party API that requires request authentication. It's a simple bearer token, which I'd like to reuse across requests, because the authentication process can take up to two seconds roughly, and obviously I don't want to flood the external service. The token is short-lived, 5 minutes only, so every 5 minutes I have to obtain a new one (unless no-one is requesting it). And during that obtaining process, I'd like to lock the process, so threads handling other requests coming in would have to wait for the first one to finish the job. My API can be quite under load, so during obtaining process (2 seconds), there could be up to a dozen or more requests coming in.

I use memory cache to store the token and share it across application. I do the double check on memory cache to filter out requests that would get into the first if statement, but would be waiting for the first request to finish obtaining a new token. That way, those requests will not trigger the token obtaining process once the wait is over, because the first request already did it.

Please take a look and tell me what do you think? Any risks, code smells? Thanks!

public class ApiTokenProvider
{
    private const string TokenCacheKey = "__TOKEN_CACHE_KEY";

    private static readonly SemaphoreSlim AuthorizationTokenSemaphore = new SemaphoreSlim(1, 1);

    private readonly IMemoryCache memoryCache;

    public ApiTokenProvider(IMemoryCache memoryCache)
    {
        this.memoryCache = memoryCache;
    }

    public async Task<string> GetToken(CancellationToken cancellationToken)
    {
        if (!memoryCache.TryGetValue<string>(TokenCacheKey, out var token))
        {
            try
            {
                await AuthorizationTokenSemaphore.WaitAsync(cancellationToken);
                if (!memoryCache.TryGetValue(TokenCacheKey, out token))
                {
                    return await CallThirdPartyApiForAuthToken(cancellationToken);
                }
            }
            finally
            {
                AuthorizationTokenSemaphore.Release();
            }
        }

        return token;
    }

    private async Task<string> CallThirdPartyApiForAuthToken(CancellationToken cancellationToken)
    {
        // Calling a third party using HttpClient. Actual code removed for brevity.
        await Task.Delay(2000, cancellationToken);

        var token = "TOKEN";
        var expireDate = DateTimeOffset.UtcNow.AddMinutes(5);
        memoryCache.Set(TokenCacheKey, token, expireDate);

        return token;
    }
}

Answer 1

Couple of things:

1. I would remove maybe 10sec from the expiration time just to be on the safe side and avoid any weird right-on-border edge cases where the token just happened to not be expired but by the time you hit the API it has. Also the clocks on your system and the 3rd party system could differ by a small portion.

2. As already mentioned in a comment - the best would probably be to have a background job updating the token every 4min30sec or so and simply replace the value unconditionally in the cache. This way you don't need a semaphore (but you probably need to acquire an initial token on startup) and there won't a be 2sec "stutter" every 5min when calling your API.

3. Consider implementing a timeout when querying the token from the external system. Simply blocking requests for a very long time isn't nice, especially since your callers may not even know about the external 3rd party service. Define a reasonable timeout (like 10sec) and return a 504 if a token cannot be obtained in that time. At least you callers will then know something is wrong rather than just being stuck.

Answer 2

One thing that I found out the hard way. Waiting for semaphore entry should be outside the try-catch statement. In original code, when a thread awaits for semaphore entry and a cancellation token is triggered Release() will be executed even though the thread did not enter the semaphore, since the waiting for entry was not finished. By executing Release() on a thread that did not enter the semaphore, it is very likely to have SemaphoreFullException being thrown, since at the time of releasing the semaphore could be already released by the other thread that was occupying it in the first place. I've experienced this first hand.

By moving the await outside the try-catch statement, execution won't reach the Release() method unless the execution has passed the WaitAsync() and we can safely assume that the currently executing thread has entered the semaphore, right? Partially. That is still a simplistic scenario.

WaitAsync() has overloads that accept time duration timeout as an alternative source of cancellation, aside from cancellation token. But what's important is that they return a bool value indicating whether entering the semaphore was actually successful. The safe approach would be to ensure that the current thread actually entered the semaphore and only then proceed. It's possible with local variable preserving the entry result. If the thread fails to enter the semaphore, it would be reasonable to notify the caller, e.g. throw an exception.

I'm using Timeout.Infinite since my cancellation token already comes from a duration-based source.

var hasEntered = await AuthorizationTokenSemaphore.WaitAsync(Timeout.Infinite, cancellationToken);
if (!hasEntered)
{
    throw new InvalidOperationException("Unable to enter token semaphore.");    
}

try
{
    if (!memoryCache.TryGetValue(TokenCacheKey, out token))
    {
        return await CallThirdPartyApiForAuthToken(cancellationToken);
    }
}
finally
{
    AuthorizationTokenSemaphore.Release();
}