Infinite loop in compiled query cache

[meikeric]

There is still an infinite loop opportunity in CompiledQueryCache.cs, RE: #7230

If the _querySyncObjects ConcurrentDictionary continuously fails to add for whatever reason, This method will never return. Some type of guard, max attempt counter, or even a timeout comparing times needs to be implemented.

Issue Core (Note the Goto-Retry Loop)

    private Func<QueryContext, TFunc> GetOrAddQueryCore<TFunc>(
        object cacheKey, Func<Func<QueryContext, TFunc>> compiler)
    {
        Func<QueryContext, TFunc> compiledQuery;

        retry:
        if (!_memoryCache.TryGetValue(cacheKey, out compiledQuery))
        {
            if (!_querySyncObjects.TryAdd(cacheKey, null))
            {
                goto retry;
            }

            try
            {
                compiledQuery = compiler();

                _memoryCache.Set(cacheKey, compiledQuery);
            }
            finally
            {
                _querySyncObjects.TryRemove(cacheKey, out _);
            }
        }

        return compiledQuery;
    }

Proposed Fix:

    private Func<QueryContext, TFunc> GetOrAddQueryCore<TFunc>(
        object cacheKey, Func<Func<QueryContext, TFunc>> compiler)
    {
        Func<QueryContext, TFunc> compiledQuery;

        const int maxAttempts = 100;

        bool syncLock = false;
        int syncLockAttempt = 0;

        if (!_memoryCache.TryGetValue(cacheKey, out compiledQuery))
        {
            do
            {
                if (_querySyncObjects.TryAdd(cacheKey, null))
                {
                    syncLock = true;
                }
            } while (!syncLock && ++syncLockAttempt < maxAttemptsk);

            if (syncLock)
            {
                try
                {
                    compiledQuery = compiler();

                    _memoryCache.Set(cacheKey, compiledQuery);
                }
                finally
                {
                    bool syncLockRemoved = false;
                    int syncRemovedAttempt = 0;

                    do
                    {
                        syncRemoved = _querySyncObjects.TryRemove(cacheKey, out _);
                    }
                    while (!syncLockRemoved && ++syncRemovedAttempt < maxAttempts);

                    if (!syncLockRemoved)
                    {
                        throw new Exception("Failure Removing Syncing Lock. Cache Timeout at MaxAttempts");
                    }
                }
            }
            else
            {
                throw new Exception("Failure Acquiring Sync Lock. Cache Timeout at MaxAttempts");
            }
            
        }

        return compiledQuery;
    }

[ajcvickers]

@meikeric Are you hitting an infinite loop, or are you just saying that it is theoretically possible? If the former, could you provide a code listing or project that demonstrates the issue? If the latter, then it seems to us that this is, from a practical perspective, impossible unless there is a bug elsewhere. This is not to say we wouldn't accept a PR for this--we would consider it--but the downside is it makes the code more complex and harder to read, and it's not clear that the upside has enough value.

[meikeric]

@ajcvickers It is theoretically.
So we can presume the implementation is safe at this time and working. For validation, I wrote a sample to stress test this, e.g. 4 threads on the NonCachingMemoryCache from the Tests just a fake CompileQueryCore with 100,000,000 loop iteration and I was NOT able to get this to infinite loop.

I would agree that there would have to be a bug upstream, either in the IMemoryCache implementation or something to cause a failure on the _querySyncObjects ConcurrentDictionary so the object can't be removed.