Fixes thread-safety issue in calculated field script evaluation (#185)

* Fixes thread-safety issue in calculated field script evaluation

Root Cause: The Jint Engine is not thread-safe. When Parallel.ForEachAsync was used to add multiple employees concurrently, multiple threads called EnsureExpressionFunction, SetSource, and GetValue simultaneously on the same engine instance, corrupting its internal parser state and causing the NullReferenceException in Acornima.Tokenizer.ReadWord1().

* Fixes jint threading issues with locking

Addresses potential race conditions in the Jint script service by ensuring proper locking around critical operations.

This change ensures that all access to shared resources within the script service is synchronized, preventing data corruption and unexpected behavior in multithreaded scenarios.

Author: niemyjski

tests/Foundatio.Repositories.Elasticsearch.Tests/Repositories/Configuration/Indexes/CalculatedIntegerFieldType.cs

@@ -1,8 +1,9 @@
-using System;
+using System;
 using System.Collections.Concurrent;
 using System.Security.Cryptography;
 using System.Text;
 using System.Threading.Tasks;
+using Foundatio.AsyncEx;
 using Foundatio.Serializer;
 using Jint;
 using Jint.Native;
@@ -20,25 +21,21 @@ public CalculatedIntegerFieldType(ScriptService scriptService)
         _scriptService = scriptService;
     }
 
-    public override Task<ProcessFieldValueResult> ProcessValueAsync<T>(T document, object value, CustomFieldDefinition fieldDefinition) where T : class
+    public override async Task<ProcessFieldValueResult> ProcessValueAsync<T>(T document, object value, CustomFieldDefinition fieldDefinition) where T : class
     {
         if (!fieldDefinition.Data.TryGetValue("Expression", out object expression))
-            return base.ProcessValueAsync(document, value, fieldDefinition);
+            return await base.ProcessValueAsync(document, value, fieldDefinition);
 
-        string functionName = _scriptService.EnsureExpressionFunction(expression.ToString());
-        _scriptService.SetSource(document);
-
-        var calculatedValue = _scriptService.GetValue(functionName);
+        var calculatedValue = await _scriptService.EvaluateForSourceAsync(document, expression.ToString());
 
         // TODO: Implement a consecutive errors counter that disables badly behaving expressions
-
         if (calculatedValue.IsCancelled)
-            return Task.FromResult(new ProcessFieldValueResult { Value = null });
+            return new ProcessFieldValueResult { Value = null };
 
-        if (calculatedValue.Value is double doubleValue && Double.IsNaN(doubleValue))
-            return Task.FromResult(new ProcessFieldValueResult { Value = null });
+        if (calculatedValue.Value is Double.NaN)
+            return new ProcessFieldValueResult { Value = null };
 
-        return Task.FromResult(new ProcessFieldValueResult { Value = calculatedValue.Value });
+        return new ProcessFieldValueResult { Value = calculatedValue.Value };
     }
 }
 
@@ -47,6 +44,7 @@ public class ScriptService
     private readonly ITextSerializer _serializer;
     private readonly ILogger<ScriptService> _logger;
     private readonly ConcurrentDictionary<string, string> _registeredExpressions = new();
+    private readonly AsyncLock _lock = new();
 
     public ScriptService(ITextSerializer jsonSerializer, ILogger<ScriptService> logger)
     {
@@ -57,20 +55,50 @@ public ScriptService(ITextSerializer jsonSerializer, ILogger<ScriptService> logg
 
     public Engine Engine { get; }
 
+    /// <summary>
+    /// Thread-safe method that atomically evaluates an expression for a given source document.
+    /// The Jint Engine is not thread-safe, so this method uses a lock to ensure only one evaluation occurs at a time.
+    /// </summary>
+    public async Task<ScriptValueResult> EvaluateForSourceAsync<T>(T source, string expression) where T : class
+    {
+        using (await _lock.LockAsync().ConfigureAwait(false))
+        {
+            string functionName = EnsureExpressionFunctionInternal(expression);
+            SetSourceInternal(source);
+            return GetValueInternal(functionName);
+        }
+    }
+
     public string EnsureExpressionFunction(string expression)
     {
-        if (_registeredExpressions.TryGetValue(expression, out var functionName))
+        using (_lock.Lock())
+        {
+            return EnsureExpressionFunctionInternal(expression);
+        }
+    }
+
+    private string EnsureExpressionFunctionInternal(string expression)
+    {
+        if (_registeredExpressions.TryGetValue(expression, out string functionName))
             return functionName;
 
         functionName = "_" + ComputeSha256Hash(expression);
-        RegisterFunction(functionName, expression);
+        RegisterFunctionInternal(functionName, expression);
 
         _registeredExpressions.TryAdd(expression, functionName);
 
         return functionName;
     }
 
     public void RegisterFunction(string name, string body)
+    {
+        using (_lock.Lock())
+        {
+            RegisterFunctionInternal(name, body);
+        }
+    }
+
+    private void RegisterFunctionInternal(string name, string body)
     {
         if (String.IsNullOrEmpty(name))
             throw new ArgumentNullException(nameof(name));
@@ -83,7 +111,7 @@ public void RegisterFunction(string name, string body)
 
         body = body.Trim();
         string script;
-        if (body.StartsWith("{"))
+        if (body.StartsWith('{'))
         {
             script = $"function {name}() {body}";
         }
@@ -100,15 +128,32 @@ public void RegisterFunction(string name, string body)
     }
 
     public void SetSource(object source)
+    {
+        using (_lock.Lock())
+        {
+            SetSourceInternal(source);
+        }
+    }
+
+    private void SetSourceInternal(object source)
     {
         string json = _serializer.SerializeToString(source);
-        if (json == null)
+        if (json is null)
             json = "null";
+
         string script = $"var source = {json};";
         Engine.Execute(script);
     }
 
     public ScriptValueResult GetValue(string functionName)
+    {
+        using (_lock.Lock())
+        {
+            return GetValueInternal(functionName);
+        }
+    }
+
+    private ScriptValueResult GetValueInternal(string functionName)
     {
         string script = $"{functionName}()";
         try
@@ -126,6 +171,14 @@ public ScriptValueResult GetValue(string functionName)
     }
 
     public object ExecuteExpression(string expression)
+    {
+        using (_lock.Lock())
+        {
+            return ExecuteExpressionInternal(expression);
+        }
+    }
+
+    private object ExecuteExpressionInternal(string expression)
     {
         if (String.IsNullOrEmpty(expression))
             throw new ArgumentNullException(nameof(expression));
@@ -140,7 +193,7 @@ public object ExecuteExpression(string expression)
         }
     }
 
-    private bool IsValidJavaScriptIdentifier(string identifier)
+    private static bool IsValidJavaScriptIdentifier(string identifier)
     {
         if (String.IsNullOrEmpty(identifier))
             return false;
@@ -152,15 +205,14 @@ private bool IsValidJavaScriptIdentifier(string identifier)
         return Array.TrueForAll(identifier.ToCharArray(), c => Char.IsLetterOrDigit(c) || c == '_');
     }
 
-    private string ComputeSha256Hash(string value)
+    private static string ComputeSha256Hash(string value)
     {
         using var sha256Hash = SHA256.Create();
-
         byte[] bytes = sha256Hash.ComputeHash(Encoding.UTF8.GetBytes(value));
 
         var builder = new StringBuilder();
-        for (int i = 0; i < bytes.Length; i++)
-            builder.Append(bytes[i].ToString("x2"));
+        foreach (byte b in bytes)
+            builder.Append(b.ToString("x2"));
 
         return builder.ToString();
     }
@@ -196,7 +248,7 @@ public ScriptValueResult(object value, bool isCancelled = false)
     public object Value { get; }
     public bool IsCancelled { get; }
 
-    public static ScriptValueResult Cancelled = new ScriptValueResult(null, true);
+    public static readonly ScriptValueResult Cancelled = new(null, true);
 }
 
 public class JintEnumConverter : IObjectConverter