7
\$\begingroup\$

I have a rest service on Azure AD that calls other rest services. They use AzureAd authentication and AuthContext with a custom TokenCache that stores each users tokens in an InMemory EF Core database. Sometimes I'd get exceptions in AfterAccessNotification when I'd call SaveChanges().

var cache = context.TokenCacheEntries.Find(userObjectId) ?? context.TokenCacheEntries
                                .Add(new TokenCacheEntry { userObjId = userObjectId }).Entity;
                cache.LastWrite = DateTime.Now;
                cache.cacheBits = Serialize();

                // update the DB and the lastwrite                
                context.SaveChanges();
                HasStateChanged = false;

A possible cause of this is I have to initialize this context in the Singleton scope because I use it via AuthContext in my JwtBearerEvents.OnTokenValidated method.

I solved the problem with a lock:

    private static object saveLock = new object();

    /// <remarks>
    /// Notification raised after ADAL accessed the cache.
    /// If the HasStateChanged flag is set, ADAL changed the content of the cache
    /// </remarks>>
    private void AfterAccessNotification(TokenCacheNotificationArgs args)
    {

        if (HasStateChanged)
        {
            // SaveChanges() threw an exception that a key already existed once so lets hope this lock fixes that.
            lock (saveLock)
            {
                // retrieve last write from the DB
                var cache = context.TokenCacheEntries.Find(userObjectId) ?? context.TokenCacheEntries
                                .Add(new TokenCacheEntry { userObjId = userObjectId }).Entity;
                cache.LastWrite = DateTime.Now;
                cache.cacheBits = Serialize();

                // update the DB and the lastwrite                
                context.SaveChanges();
                HasStateChanged = false;
            }
        }
    }

Is there a better way?

\$\endgroup\$
1
  • 1
    \$\begingroup\$ A lock works pretty well here, assuming you don't have multiple users with a need to call to it at once. You can also catch the exception being thrown and recheck to see if there's a valid token in the cache. \$\endgroup\$ Commented May 3, 2018 at 18:08

1 Answer 1

Reset to default
7
\$\begingroup\$

A lock works here, and it's functional, but it does come with some drawbacks in this case.

If you and I both submit the request at the same time, one of them will be delayed even though there wasn't actually a reason. Now you've introduced a potentially hazardous situation, what happens if a lot of distinct persons submit requests at once?

The best option here, is to block only those who violated the key sanity. The easiest way to do this is actually to catch the appropriate exception.

There's a nice question about this on Stack Overflow, which I would like to borrow some information from: Duplicate key exception from Entity Framework?

There is an answer that looks promising, which looks like:

try
{
    await db.SaveChangesAsync();
}
catch (DbUpdateException ex)
{
    SqlException innerException = ex.InnerException.InnerException as SqlException;
    if (innerException != null && (innerException.Number == 2627 || innerException.Number == 2601))
    {
        //your handling stuff
    }
    else
    {
        throw;
    }
}

Based on the research I could suss-out in a short bit, this looks like it's appropriate for EF-Core, whereas the accepted answer looks more like EF on standard. I can't test it, so I leave it to you, but I would go more towards this route than a shared lock.

Of course, it's up to you, but I wouldn't even try to re-save the LastWrite on a duplicate key violation, because we know it's going to be right now, so why do the extra work? I'd just catch and swallow in this case, with a clear comment as to why:

try
{
    context.SaveChanges();
}
catch (DbUpdateException ex)
{
    SqlException innerException = ex.InnerException.InnerException as SqlException;
    if (innerException != null && (innerException.Number == 2627 || innerException.Number == 2601))
    {
        // Swallow the exception, as the only thing we're updating is the `LastWrite`, which will be the current date/time so there's not point in doing additional queries;
    }
    else
    {
        throw;
    }
}

Depending on the logic, I may also even recommend tracking that further up the stack, and only call to this function if it needs updated. (Rather than for every update.)

\$\endgroup\$
3
  • \$\begingroup\$ Thanks. Another curious thought, what if i had a dictionary of lock objects keyed to the user id and effectively only locked per user? Another part of the problem is I never actually call Acquire TokrnSilent() so I'm always writing new keys. Actually reading from the cache likely makes this a rare occurrence. And yes it would have to be some sort of cached dictionary of lock objects where the keys expire. \$\endgroup\$ Commented May 3, 2018 at 20:22
  • \$\begingroup\$ Oh you can for sure. You'll still lose some performance for each lock but that should prevent the lock from interfering with other sessions. You can always Dictionary<Token, object>. You can also use a more sophisticated option like this other question does. \$\endgroup\$ Commented May 3, 2018 at 20:27
  • \$\begingroup\$ While this answer solves the OP's issue, it raises a lot of other hell with it. \$\endgroup\$ Commented Jul 9, 2019 at 19:08

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.