Caching data by using the result of first running operation

My code handles concurrent requests by waiting for the result of an already running operation. Requests for data may come in simultaneously with same/different credentials (including empty credentials).

For each unique set of credentials there can be at most one GetCurrentInternal call in progress, with the result from that one call returned to all queued waiters when it is ready.

private static readonly Credentials EmptyCredentials = new Credentials
{
    SqlCredentials = null,
    ExchangeCredentials = null,
};

public AgentMetadata GetCurrent(Credentials credentials)
{
    var agentMetadata = new Lazy(() => GetCurrentInternal(credentials));

    var lazyMetadata = (Lazy)MemoryCache.Default.AddOrGetExisting(
                       (credentials ?? EmptyMetadataCredentials).ToString(),
                       agentMetadata,
                       DateTime.Now.AddMinutes(5));

    try
    {
        return (lazyMetadata ?? agentMetadata).Value;
    }
    catch (InvalidOperationException ex)
    {
        _logger.ErrorException(ex, "An error occurs during getting full metadata");
        throw;
    }
}

Is it thread safe? Do I have a bugs? How to create a good key for the AddOrGetExisting?

Credentials its a DataContract that contains other DataContract like SqlCredentials, ExchangeCredentials. And they contains their own strings like user_name and password.

Yes, it seems thread safe.

I run a test to prove it which I give the details at the bottom, but first are some code review comments.

-
The code is missing the Credentials class which I think is also important for the review. The cache items are added with keys credentials.ToString() and it is important for this method to return the same value for Credentials objects having the same credential values, and distinct values for Credentials instances with different credential values. Is it?

-
Naming conventions. Class names should be PascalCase and the class AgentMetadata violates this rule. Should be AgentMetaData

-
To achieve better encapsulation and seperation of concerns, (The creation of an empty credential object should be a concern for the Credentials class, which has control over the internals of itself) instead of having a static readonly Credentials EmptyCredentials field in this class, it is better to have a public static readonly Credentials Empty field (or property getter) in Credentials class.

Instead of this:

Credentials empty = (credentials ?? EmptyMetadataCredentials);

Isn't it better to have this?

Credentials empty = (credentials ?? Credentials.Empty);

• The variable names agentMetadata and lazyMetadata are not descriptive enough.

This is somewhat confusing:

return (lazyMetadata ?? agentMetadata).Value;

Having names like the following might eliminate that confusion:

var newLazyInstance = new Lazy(() => GetCurrentInternal(credentials));

var cachedLazyInstance = (Lazy)MemoryCache.Default.AddOrGetExisting(
                           (credentials ?? EmptyMetadataCredentials).ToString(),
                           newLazyInstance,
                           DateTime.Now.AddMinutes(5));

return (cachedLazyInstance ?? newLazyInstance).Value;

And here are the results of the test, which shows the GetCurrentInternal method is invoked a total of 5 times for all the 100 threads.

The test created 100 threads which access the GetCurrent method randomly within 500 ms. There is another thread for removing the item from the cache in 100 ms continuously (This is because, the cache policy doesn't seem to effectively remove the item at the exact point of time of expiration).

The GetCurrentInternal method lasts for a random time between 0 and 500 ms

With the following results, it can be seen that the GetCurrentInternal method is invoked a total of 5 times (which is the number of the cache not containing a Lazy instance for the given credentials, first one because the cache is empty, and for 4 more times because it is removed from the cache)

I can't help myself thinking why the AddOrGetExisting of MemoryCache returns null when the item is added to the cache. This would have been implemented differently (as returning the added instance as ConcurrentDictionary does)

```
T:47 M:GetCurrent E:AddOrGetExisting.Added? R:False 145,68
T:13 M:GetCurrent E:AddOrGetExisting.Added? R:False 146,44
T:20 M:GetCurrent E:AddOrGetExisting.Added? R:False 146,54
T:26 M:GetCurrent E:AddOrGetExisting.Added? R:False 147,45
T:71 M:GetCurrent E:AddOrGetExisting.Added? R:False 148,46
T:62 M:GetCurrent E:AddOrGetExisting.Added? R:False 148,55
T:53 M:GetCurrent E:AddOrGetExisting.Added? R:False 149,37
T:5 M:GetCurrent E:AddOrGetExisting.Added? R:False 146,68
T:22 M:GetCurrent E:AddOrGetExisting.Added? R:True 150,66
T:31 M:GetCurrent E:AddOrGetExisting.Added? R:False 150,18
T:63 M:GetCurrent E:AddOrGetExisting.Added? R:False 154,58
T:50 M:GetCurrent E:AddOrGetExisting.Added? R:False 157,15
T:27 M:GetCurrent E:AddOrGetExisting.Added? R:False 182,13
T:67 M:GetCurrent E:AddOrGetExisting.Added? R:False 182,33
T:43 M:GetCurrent E:AddOrGetExisting.Added? R:False 182,18
T:32 M:GetCurrent E:AddOrGetExisting.Added? R:False 182,13
T:3 M:GetCurrent E:AddOrGetExisting.Added? R:False 187,24
T:55 M:GetCurrent E:AddOrGetExisting.Added? R:False 194,8
T:29 M:GetCurrent E:AddOrGetExisting.Added? R:False 197,13
T:28 M:GetCurrent E:AddOrGetExisting.Added? R:False 214,17
T:4 M:GetCurrent E:AddOrGetExisting.Added? R:False 221,11
T:84 M:GetCurrent E:AddOrGetExisting.Added? R:False 228,12
T:78 M:GetCurrent E:AddOrGetExisting.Added? R:False 232,09
T:19 M:GetCurrent E:AddOrGetExisting.Added? R:False 235,1
T:66 M:GetCurrent E:AddOrGetExisting.Added? R:False 238,1
T:7 M:GetCurrent E:AddOrGetExisting.Added? R:False 242,14
T:61 M:GetCurrent E:AddOrGetExisting.Added? R:False 245,11
T:49 M:GetCurrent E:AddOrGetExisting.Added? R:False 246,15
T:23 M:GetCurrent E:AddOrGetExisting.Added? R:False 250,08
T:85 M:GetCurrent E:AddOrGetExisting.Added? R:False 251,1
T:81 M:GetCurrent E:AddOrGetExisting.Added? R:False 254,1
T:98 M:GetCurrent E:AddOrGetExisti

StackExchange Code Review Q#115047, answer score: 4