Race Condition in ConstantExpressionHelper Causing Parsing Failures

[RenanCarlosPereira]

Issue Summary

We encountered a race condition in ConstantExpressionHelper that can lead to parsing failures in ExpressionPromoter when _literals is cleaned up.
This issue is caused by SlidingCache, which automatically removes expired items, making Promote() unable to retrieve the required text representation of constants.

How This Issue Occurs

1. A thread calls CreateLiteral(value, text), storing:

   • _expressions.AddOrUpdate(value, constantExpression);
   • _literals.AddOrUpdate(constantExpression, text);

2. Another thread runs cache cleanup, removing _literals and _expressions due to the CleanupFrequency and TimeToLive properties in ConstantExpressionCacheConfig.

3. A third thread calls Promote(), which attempts to retrieve the text using:

   _constantExpressionHelper.TryGetText(ce, out var text);

   • Since _literals was cleaned, TryGetText() returns false, and Promote() fails to perform the type promotion.

4. This leads to an error when trying to compare a double with a decimal, causing:

   Exception while parsing expression `Variable < 0.40` - 
   Operator '<' incompatible with operand types 'Decimal?' and 'Double'
   

---

Reproduction

We created a unit test that simulates this race condition:

• Test Name: Promote_Should_Succeed_Even_When_LiteralsCache_Is_Cleaned

• Steps:

  1. Add a constant to the cache.
  2. Simulate cache cleanup by repeatedly calling TryGetText() until _literals is cleaned.
  3. Attempt to Promote() the expression after cleanup.
  4. Expected: Promote() should still return a valid expression (not null).

• Without Fix: The test fails due to missing _literals entries.

• With Fix: Promote() properly handles the absence of _literals.

---

Proposed Fix

1. Ensure Promote() does not depend entirely on _literals:

   • If TryGetText() fails, fallback to _expressions or recompute the value.

2. Prevent aggressive cleanup of _literals:

   • Increase _literals TTL.
   • Keep _expressions and _literals in sync.

3. Lazy loading mechanism for _literals:

   • If _literals is empty, reconstruct the text from _expressions.

---

Next Steps

1. Review and confirm the issue using the provided test.
2. Discuss the best approach for fixing this issue.
3. Implement the fix and verify with tests.

    [Fact]
    public async Task Promote_Should_Succeed_Even_When_LiteralsCache_Is_Cleaned()
    {
        // Arrange
        var parsingConfig = new ParsingConfig()
        {
            ConstantExpressionCacheConfig = new Core.Util.Cache.CacheConfig
            {
                CleanupFrequency = TimeSpan.FromMilliseconds(500), // Run cleanup more often
                TimeToLive = TimeSpan.FromMilliseconds(500), // Shorten TTL to force expiration
                ReturnExpiredItems = false
            }
        };
        var constantExpressionHelper = ConstantExpressionHelperFactory.GetInstance(parsingConfig);
        var expressionPromoter = new ExpressionPromoter(parsingConfig);

        double value = 0.40;
        string text = "0.40";
        Type targetType = typeof(decimal);

        // Step 1: Add constant to cache
        var literalExpression = constantExpressionHelper.CreateLiteral(value, text);
        Assert.NotNull(literalExpression); // Ensure it was added

        // Step 2: Manually trigger cleanup
        var cts = new CancellationTokenSource(500);
        await Task.Run(async () =>
        {
            while (!cts.IsCancellationRequested)
            {
                constantExpressionHelper.TryGetText(literalExpression, out _);
                await Task.Delay(50); // Give some time for cleanup to be triggered
            }
        });

        // Ensure some cleanup cycles have passed
        await Task.Delay(500); // Allow cache cleanup to happen

        // Step 3: Attempt to promote the expression after cleanup
        var promotedExpression = expressionPromoter.Promote(literalExpression, targetType, exact: false, convertExpression: true);

        // Assert: Promotion should still work even if the cache was cleaned
        promotedExpression.Should().NotBeNull(); // Ensure `Promote()` still returns a valid expression
    }

Would love feedback on the best approach! 🚀

[RenanCarlosPereira]

Note: this is an example where this could go wrong between expression1 and expression2, if time passes and other threads are happening, and the cache is cleaned.

var config = new ParsingConfig();

var item = new { A = 1m, B = 2m };
var strExp = "(item.A / item.B) < 0.6";

var pa = new[] { Expression.Parameter(item.GetType(), "item") };
var expression1 = new ExpressionParser(pa, strExp, [], config).Parse(typeof(bool));
var expression2 = new ExpressionParser(pa, strExp, [], config).Parse(typeof(bool));

[RenanCarlosPereira]

it might be related to this one #769

[RenanCarlosPereira]

Opened the pull request #905 please take a look 👍

[grichards23]

I definitely feel like we're running into this in our implementation. We've been chasing for a couple of hours.
Eager to try to apply this fix and see if helps, any idea when the PR will be approved and a patch version will be out?

Thank you!

[RenanCarlosPereira]

I definitely feel like we're running into this in our implementation. We've been chasing for a couple of hours. Eager to try to apply this fix and see if helps, any idea when the PR will be approved and a patch version will be out?

Thank you!

One thing that you could try to do is set the CleanupFrequency and TimeToLive in the ConstantExpressionCacheConfig for, let's say 10 years, that will solve the problem.

but the dictionary will never be clear, so depending on how much constants you have in your expressions, you will be seeing a memory leak

[RenanCarlosPereira]

@StefH would you mind creating a version?