From 4e82c672eb601fc1e9e031fcfc944bccd8048502 Mon Sep 17 00:00:00 2001 From: Trung Tran Date: Sat, 22 Jun 2024 15:06:49 +0800 Subject: [PATCH] Add Task async await check when evaluate await expression (#1882) * Fix event loop using concurrent queue * Add continuation task fix * Enhance continuation task * Remove wait in event loop --------- Co-authored-by: Trung Tran Co-authored-by: Ngoc.TranThi --- Jint.Tests/Runtime/AsyncTests.cs | 71 ++++++++++++++++++++++++++++++++ Jint/Engine.cs | 17 ++------ Jint/JsValueExtensions.cs | 12 +----- Jint/Native/JsPromise.cs | 7 ++-- Jint/Native/JsValue.cs | 4 +- Jint/Runtime/EventLoop.cs | 4 +- 6 files changed, 86 insertions(+), 29 deletions(-) diff --git a/Jint.Tests/Runtime/AsyncTests.cs b/Jint.Tests/Runtime/AsyncTests.cs index c83ab73d4e..da31e21332 100644 --- a/Jint.Tests/Runtime/AsyncTests.cs +++ b/Jint.Tests/Runtime/AsyncTests.cs @@ -1,3 +1,5 @@ +using System.Collections.Concurrent; +using Jint.Native; using Jint.Tests.Runtime.TestClasses; namespace Jint.Tests.Runtime; @@ -278,4 +280,73 @@ async function main() { var val = result.GetValue("main").Call(); Assert.Equal(1, val.UnwrapIfPromise().AsInteger()); } + + [Fact] + public async Task ShouldEventLoopBeThreadSafeWhenCalledConcurrently() + { + const int ParallelCount = 1000; + + // [NOTE] perform 5 runs since the bug does not always happen + for (int run = 0; run < 5; run++) + { + var tasks = new List>(); + + for (int i = 0; i < ParallelCount; i++) + tasks.Add(new TaskCompletionSource()); + + for (int i = 0; i < ParallelCount; i++) + { + int taskIdx = i; + _ = Task.Factory.StartNew(() => + { + try + { + Engine engine = new(options => options.ExperimentalFeatures = ExperimentalFeature.TaskInterop); + + const string Script = """ + async function main(testObj) { + async function run(i) { + await testObj.Delay(100); + await testObj.Add(`${i}`); + } + + const tasks = []; + for (let i = 0; i < 10; i++) { + tasks.push(run(i)); + } + for (let i = 0; i < 10; i++) { + await tasks[i]; + } + return 1; + } + """; + var result = engine.Execute(Script); + var testObj = JsValue.FromObject(engine, new TestAsyncClass()); + var val = result.GetValue("main").Call(testObj); + tasks[taskIdx].SetResult(null); + } + catch (Exception ex) + { + tasks[taskIdx].SetException(ex); + } + }, creationOptions: TaskCreationOptions.LongRunning); + } + + await Task.WhenAll(tasks.Select(t => t.Task)); + await Task.Delay(100); + } + } + + class TestAsyncClass + { + private readonly ConcurrentBag _values = new(); + + public Task Delay(int ms) => Task.Delay(ms); + + public Task Add(string value) + { + _values.Add(value); + return Task.CompletedTask; + } + } } diff --git a/Jint/Engine.cs b/Jint/Engine.cs index da0f7ed2fd..3c50a94e85 100644 --- a/Jint/Engine.cs +++ b/Jint/Engine.cs @@ -1,4 +1,5 @@ using System.Diagnostics; +using System.Collections.Concurrent; using System.Diagnostics.CodeAnalysis; using System.Runtime.CompilerServices; using Jint.Native; @@ -508,25 +509,13 @@ internal void AddToKeptObjects(JsValue target) internal void RunAvailableContinuations() { var queue = _eventLoop.Events; - if (queue.Count == 0) - { - return; - } - DoProcessEventLoop(queue); } - private static void DoProcessEventLoop(Queue queue) + private static void DoProcessEventLoop(ConcurrentQueue queue) { - while (true) + while (queue.TryDequeue(out var nextContinuation)) { - if (queue.Count == 0) - { - return; - } - - var nextContinuation = queue.Dequeue(); - // note that continuation can enqueue new events nextContinuation(); } diff --git a/Jint/JsValueExtensions.cs b/Jint/JsValueExtensions.cs index 04e7e44b3d..62112eed33 100644 --- a/Jint/JsValueExtensions.cs +++ b/Jint/JsValueExtensions.cs @@ -650,17 +650,9 @@ public static JsValue UnwrapIfPromise(this JsValue value) if (value is JsPromise promise) { var engine = promise.Engine; - var task = promise.TaskCompletionSource.Task; - engine.RunAvailableContinuations(); - engine.AddToEventLoop(() => - { - if (!task.IsCompleted) - { - // Task.Wait has the potential of inlining the task's execution on the current thread; avoid this. - ((IAsyncResult) task).AsyncWaitHandle.WaitOne(); - } - }); + var completedEvent = promise.CompletedEvent; engine.RunAvailableContinuations(); + completedEvent.Wait(); switch (promise.State) { case PromiseState.Pending: diff --git a/Jint/Native/JsPromise.cs b/Jint/Native/JsPromise.cs index 4258ab1bc8..d6f1473cb2 100644 --- a/Jint/Native/JsPromise.cs +++ b/Jint/Native/JsPromise.cs @@ -1,3 +1,4 @@ +using System.Threading; using Jint.Native.Object; using Jint.Native.Promise; using Jint.Runtime; @@ -12,7 +13,7 @@ internal sealed class JsPromise : ObjectInstance // valid only in settled state (Fulfilled or Rejected) internal JsValue Value { get; private set; } = null!; - internal TaskCompletionSource TaskCompletionSource { get; }= new(); + internal ManualResetEventSlim CompletedEvent { get; } = new(); internal List PromiseRejectReactions = new(); internal List PromiseFulfillReactions = new(); @@ -127,7 +128,7 @@ private JsValue RejectPromise(JsValue reason) var reactions = PromiseRejectReactions; PromiseRejectReactions = new List(); PromiseFulfillReactions.Clear(); - TaskCompletionSource.SetCanceled(); + CompletedEvent.Set(); // Note that this part is skipped because there is no tracking yet // 7. If promise.[[PromiseIsHandled]] is false, perform HostPromiseRejectionTracker(promise, "reject"). @@ -147,7 +148,7 @@ private JsValue FulfillPromise(JsValue result) var reactions = PromiseFulfillReactions; PromiseFulfillReactions = new List(); PromiseRejectReactions.Clear(); - TaskCompletionSource.SetResult(this); + CompletedEvent.Set(); return PromiseOperations.TriggerPromiseReactions(_engine, reactions, result); } diff --git a/Jint/Native/JsValue.cs b/Jint/Native/JsValue.cs index 5986a2d75f..7c3ba70232 100644 --- a/Jint/Native/JsValue.cs +++ b/Jint/Native/JsValue.cs @@ -176,7 +176,9 @@ internal static JsValue ConvertTaskToPromise(Engine engine, Task task) resolve(FromObject(engine, JsValue.Undefined)); } } - }); + }, + // Ensure continuation is completed before unwrapping Promise + continuationOptions: TaskContinuationOptions.AttachedToParent | TaskContinuationOptions.ExecuteSynchronously); return promise; } diff --git a/Jint/Runtime/EventLoop.cs b/Jint/Runtime/EventLoop.cs index ccdba2b3e4..3c9a288dcc 100644 --- a/Jint/Runtime/EventLoop.cs +++ b/Jint/Runtime/EventLoop.cs @@ -1,7 +1,9 @@ +using System.Collections.Concurrent; + namespace Jint.Runtime { internal sealed record EventLoop { - internal readonly Queue Events = new(); + internal readonly ConcurrentQueue Events = new(); } }