Add Task async await check when evaluate await expression

[trannamtrung1st]

Hi team, this PR is my idea to support await an async method from C# object. Before, I cannot await an async result when calling async function from a C# object. It always returns Task. Now I can make it works like below:

const seriesTask = InputMetric.LastSeriesBefore(BeforeTime); // async method that will return Task<Series>
FB.Log('Checkpoint 1'); // Log immediately after LastSeriesBefore (it won't block)
const undefinedValue = await FB.DelayAsync(5000); // actually delay 5s
FB.Log(undefinedValue, 'Checkpoint 2'); // since delay return undefined so it just logs the Checkpoint 2
FB.Log(seriesTask); // wrapper class for C# Task API
FB.Log(await seriesTask); // My series object will be returned after 10s (for demo) since its invocation

Sorry for my English and since I don't have much time to research all the implementation here, so welcome any feedback you may have.

Thanks! Trung Tran.

[trannamtrung1st]

After doing some experiments I think this async/await case only failed if I called an async method from C# object.

[trannamtrung1st]

Hi @sebastienros @lahma, it would be great if you guys could take a look at this. It is more like fixing a bug I found while using it for my project. Thanks.

[lahma]

@trannamtrung1st have your cheked: https://github.com/sebastienros/jint/blob/main/Jint.Tests/Runtime/AsyncTests.cs and how options.ExperimentalFeatures = ExperimentalFeature.TaskInterop correlates to your needs?

[trannamtrung1st]

@trannamtrung1st have your cheked: https://github.com/sebastienros/jint/blob/main/Jint.Tests/Runtime/AsyncTests.cs and how options.ExperimentalFeatures = ExperimentalFeature.TaskInterop correlates to your needs?

Wonderful that's exactly what I want. Many thanks.

The only problem left is this one, I believe it is because there're some functions return Task but not actually async, e.g, Task.CompletedTask or Task.FromResult(...). Therefore the Task.ContinueWith callback is not triggered here: (https://github.com/sebastienros/jint/blob/c0b0369dd4b76f8d1485dd0c8c7db368c197dadc/Jint/Native/JsValue.cs#L149) It does not always happen, I found it while trying to create about 1000 threads (each with their own Engine) to simulate some scenarios.

Here the Microsoft's documentation link about the ContinueWith: https://learn.microsoft.com/en-us/dotnet/api/system.threading.tasks.task.continuewith?view=net-8.0#system-threading-tasks-task-continuewith(system-action((system-threading-tasks-task-system-object))-system-object-system-threading-cancellationtoken-system-threading-tasks-taskcontinuationoptions-system-threading-tasks-taskscheduler)

I already removed all unused part in the PR except for the one is to fix the above issue. You may want to take a look. Thanks for your quick reply!

[trannamtrung1st]

Wait a second. Sorry for the confusion but I just tested the ContinueWith, seems like it's not the cause. However I'm not sure why when I apply the fix, it solves the issue. I'll try to investigate it more.

[trannamtrung1st]

Hi @lahma, PR updated, below is the explanation

[lahma]

Could you add some test case(s) that would ensure that the behavior has been corrected and will stay working as expected?

[trannamtrung1st]

Could you add some test case(s) that would ensure that the behavior has been corrected and will stay working as expected?

Hi @lahma , Sure. I've added 5 new commits, you can verify it on your side by switching to these commits below:

• Add test and remove fix - b0388a1f6: This one contains the test without any fix, it should throw either of these exceptions: Object null, Queue empty or UnwrapIfPromise called before completed
• Bring back fix to pass the test - 4054ac36c: This one is to add ConcurrentQueue to solve the concurrent issue when accessing Queue. It should prevent Object null and Queue empty exceptions, but I found the UnwrapIfPromise error arose sometimes.
• Add continuation task fix - 5ac14c273 and Enhance continuation task - 14206a9fd: These twos are to make sure the task continuation is finished before unwrapping the promise in JsValueExtensions here: https://github.com/sebastienros/jint/blob/c0b0369dd4b76f8d1485dd0c8c7db368c197dadc/Jint/JsValueExtensions.cs#L664
• Refactor test case - a23cc40f7: Final test case refactoring to enhance exception report

[lahma]

D:\a\jint\jint\Jint.Tests\Runtime\AsyncTests.cs(292,34): error CS0305: Using the generic type 'TaskCompletionSource' requires 1 type arguments [D:\a\jint\jint\Jint.Tests\Jint.Tests.csproj::TargetFramework=net462]

[trannamtrung1st]

D:\a\jint\jint\Jint.Tests\Runtime\AsyncTests.cs(292,34): error CS0305: Using the generic type 'TaskCompletionSource' requires 1 type arguments [D:\a\jint\jint\Jint.Tests\Jint.Tests.csproj::TargetFramework=net462]

Oh I used a Mac before so I missed that one. Thanks. PR updated.

[trannamtrung1st]

Seems that net462 on Windows behaves in a slightly different way. I will have to spend a bit more time to investigate.

[lahma]

Windows run fails:

Error: System.InvalidOperationException : 'UnwrapIfPromise' called before Promise was settled

  Failed Jint.Tests.Runtime.AsyncTests.ShouldEventLoopBeThreadSafeWhenCalledConcurrently [7 s]
  Error Message:
   System.InvalidOperationException : 'UnwrapIfPromise' called before Promise was settled
  Stack Trace:
     at Jint.Runtime.ExceptionHelper.ThrowInvalidOperationException(String message, Exception exception) in D:\a\jint\jint\Jint\Runtime\ExceptionHelper.cs:line 133
   at Jint.JsValueExtensions.UnwrapIfPromise(JsValue value) in D:\a\jint\jint\Jint\JsValueExtensions.cs:line 667
   at Jint.Runtime.Interpreter.Expressions.JintAwaitExpression.EvaluateInternal(EvaluationContext context) in D:\a\jint\jint\Jint\Runtime\Interpreter\Expressions\JintAwaitExpression.cs:line 37
   at Jint.Runtime.Interpreter.Expressions.JintExpression.GetValue(EvaluationContext context) in D:\a\jint\jint\Jint\Runtime\Interpreter\Expressions\JintExpression.cs:line 26
   at Jint.Runtime.Interpreter.Statements.JintExpressionStatement.ExecuteInternal(EvaluationContext context) in D:\a\jint\jint\Jint\Runtime\Interpreter\Statements\JintExpressionStatement.cs:line 24
   at Jint.Runtime.Interpreter.JintFunctionDefinition.<>c__DisplayClass9_0.<EvaluateBody>b__1(EvaluationContext context) in D:\a\jint\jint\Jint\Runtime\Interpreter\JintFunctionDefinition.cs:line 77
   at Jint.Runtime.Interpreter.JintFunctionDefinition.AsyncBlockStart(EvaluationContext context, PromiseCapability promiseCapability, Func`2 asyncBody, ExecutionContext& asyncContext) in D:\a\jint\jint\Jint\Runtime\Interpreter\JintFunctionDefinition.cs:line 119
   at Jint.Native.Function.ScriptFunction.Call(JsValue thisObject, JsValue[] arguments) in D:\a\jint\jint\Jint\Native\Function\ScriptFunction.cs:line 78
   at Jint.Runtime.Interpreter.Expressions.JintCallExpression.EvaluateInternal(EvaluationContext context) in D:\a\jint\jint\Jint\Runtime\Interpreter\Expressions\JintCallExpression.cs:line 192
   at Jint.Runtime.Interpreter.Expressions.JintExpression.GetValue(EvaluationContext context) in D:\a\jint\jint\Jint\Runtime\Interpreter\Expressions\JintExpression.cs:line 26
   at Jint.Runtime.Interpreter.Expressions.JintCallExpression.ArgumentListEvaluation(EvaluationContext context) in D:\a\jint\jint\Jint\Runtime\Interpreter\Expressions\JintCallExpression.cs:line 316
   at Jint.Runtime.Interpreter.Expressions.JintCallExpression.EvaluateInternal(EvaluationContext context) in D:\a\jint\jint\Jint\Runtime\Interpreter\Expressions\JintCallExpression.cs:line 157
   at Jint.Runtime.Interpreter.Expressions.JintExpression.GetValue(EvaluationContext context) in D:\a\jint\jint\Jint\Runtime\Interpreter\Expressions\JintExpression.cs:line 26
   at Jint.Runtime.Interpreter.Statements.JintExpressionStatement.ExecuteInternal(EvaluationContext context) in D:\a\jint\jint\Jint\Runtime\Interpreter\Statements\JintExpressionStatement.cs:line 24
   at Jint.Runtime.Interpreter.Statements.JintBlockStatement.ExecuteSingle(EvaluationContext context) in D:\a\jint\jint\Jint\Runtime\Interpreter\Statements\JintBlockStatement.cs:line 72
   at Jint.Runtime.Interpreter.Statements.JintBlockStatement.ExecuteBlock(EvaluationContext context) in D:\a\jint\jint\Jint\Runtime\Interpreter\Statements\JintBlockStatement.cs:line 52
   at Jint.Runtime.Interpreter.Statements.JintForStatement.ForBodyEvaluation(EvaluationContext context) in D:\a\jint\jint\Jint\Runtime\Interpreter\Statements\JintForStatement.cs:line 138
   at Jint.Runtime.Interpreter.Statements.JintForStatement.ExecuteInternal(EvaluationContext context) in D:\a\jint\jint\Jint\Runtime\Interpreter\Statements\JintForStatement.cs:line 101
   at Jint.Runtime.Interpreter.JintFunctionDefinition.<>c__DisplayClass9_0.<EvaluateBody>b__1(EvaluationContext context) in D:\a\jint\jint\Jint\Runtime\Interpreter\JintFunctionDefinition.cs:line 77
   at Jint.Runtime.Interpreter.JintFunctionDefinition.AsyncBlockStart(EvaluationContext context, PromiseCapability promiseCapability, Func`2 asyncBody, ExecutionContext& asyncContext) in D:\a\jint\jint\Jint\Runtime\Interpreter\JintFunctionDefinition.cs:line 119
   at Jint.Native.Function.ScriptFunction.Call(JsValue thisObject, JsValue[] arguments) in D:\a\jint\jint\Jint\Native\Function\ScriptFunction.cs:line 78
   at Jint.Engine.Call(Function function, JsValue thisObject, JsValue[] arguments, JintExpression expression) in D:\a\jint\jint\Jint\Engine.cs:line 1519
   at Jint.Engine.<>c__DisplayClass136_0.<Call>g__Callback|0() in D:\a\jint\jint\Jint\Engine.cs:line 1433
   at Jint.Engine.ExecuteWithConstraints[T](Boolean strict, Func`1 callback) in D:\a\jint\jint\Jint\Engine.cs:line 807
   at Jint.Engine.Call(JsValue callable, JsValue thisObject, JsValue[] arguments) in D:\a\jint\jint\Jint\Engine.cs:line 1436
   at Jint.Engine.Call(JsValue callable, JsValue[] arguments) in D:\a\jint\jint\Jint\Engine.cs:line 1415
   at Jint.JsValueExtensions.Call(JsValue value, JsValue arg1) in D:\a\jint\jint\Jint\JsValueExtensions.cs:line 563
   at Jint.Tests.Runtime.AsyncTests.<>c__DisplayClass16_1.<ShouldEventLoopBeThreadSafeWhenCalledConcurrently>b__1() in D:\a\jint\jint\Jint.Tests\Runtime\AsyncTests.cs:line 325
--- End of stack trace from previous location ---
   at Jint.Tests.Runtime.AsyncTests.ShouldEventLoopBeThreadSafeWhenCalledConcurrently() in D:\a\jint\jint\Jint.Tests\Runtime\AsyncTests.cs:line 335
--- End of stack trace from previous location ---

I'll be travelling and back tomorrow evening..

[trannamtrung1st]

Hi @lahma, I've tried running test multiple times in a Windows 11 machine using net462 test cases but cannot reproduce it. I also looked at the code and see nothing's wrong with it in the UnwrapIfPromise method: https://github.com/sebastienros/jint/blob/c0b0369dd4b76f8d1485dd0c8c7db368c197dadc/Jint/JsValueExtensions.cs#L657

However, I feel like the TaskCompletionSource is the cause here due to it's complicated internal implementation to support Tasks, ThreadPool, etc. There're maybe some compatible issues between framework version as well. Apart from that, I don't see any other specific usage of TaskCompletionSource in the code except for waiting the JsPromise to complete. Therefore, I've tried replacing it with a lightweight ManualResetEventSlim to fit its purpose.

Changes are also tested in a Windows 11 machine with net462 framework. Could you help trigger the test for Windows net462 again? This is kinda weird.

Please take a look at this commit: https://github.com/sebastienros/jint/pull/1882/commits/6d02edc9623599b41c698a27c01705de5048dc1b

[trannamtrung1st]

Hi @lahma , is there any update on this one? It would be great to see the Tasks Interop feature become mature and included in the official package so we can use it without cloning one.

[trannamtrung1st]

Honestly, I have no idea why it failed on Windows since I couldn't replicate it in the local. The only situation I can think of now is there are other async codes/threads accessing the queue and executing the Wait there before this line engine.RunAvailableContinuations(); runs in here to block the UnwrapIfPromise https://github.com/trannamtrung1st/jint/blob/6d02edc9623599b41c698a27c01705de5048dc1b/Jint/JsValueExtensions.cs#L656

Do you mind if I add one more commit to try solving it? So inconvenient that I couldn't replicate it locally.

[lahma]

You can work with the PR as much as you want. I'll try to trigger builds when I'm available.

[trannamtrung1st]

Thanks, that's great. However, after carefully checking, I think the way we're doing the event loop check is a bit inflexible. I would love to hear your thoughts on this before I do any further work.

Let's take this example:

async function delay(ms) => await DelayAsync(ms);

const t1 = delay(10);
const t2 = delay(10);
const t3 = delay(10);

await t1; await t2; await t3;

These delay calls trigger the DelayAsync promises 3 times which are then unwrapped and will wait for the event loop processing almost at the same time. This makes the event loop not single-threaded anymore, even when I used ConcurrentQueue, the actual continuation can run on a different thread from the expected one. If I try to lock the loop execution, it causes deadlock since a new action can be enqueued and wait for the event loop again which causes cyclic dependency.

I may close this PR since it won't solve the problem completely. Can we discuss the solution here? I will be happy to give a hand on doing this one if possible.

Currently, event loop is triggered in many places (call RunAvailableContinuations), while I thought it should only be triggered after the last call stack is popped and the stack is empty (to ensure it's single-threaded). C# async, await is just an async operation and its callback (or await) will be placed back into the callback queue (see below flow)

1. Call Stack: JavaScript uses a call stack to keep track of the currently executing function (where the program is in its execution).
2. Callback Queue: Asynchronous operations, such as I/O operations or timers, are handled by the browser or Node.js runtime. When these operations are complete, corresponding functions (callbacks) are placed in the callback queue.
3. Event Loop: The event loop continuously checks the call stack and the callback queue. If the call stack is empty, it takes the first function from the callback queue and pushes it onto the call stack for execution.
4. Execution: The function on top of the call stack is executed. If this function contains asynchronous code, it might initiate further asynchronous operations.
5. Callback Execution: When an asynchronous operation is complete, its callback is placed in the callback queue.
6. Repeat: The event loop continues this process, ensuring that the call stack is always empty before taking the next function from the callback queue.

References:

• https://www.geeksforgeeks.org/what-is-an-event-loop-in-javascript/
• https://stackoverflow.com/questions/46004290/will-async-await-block-a-thread-node-js

[lahma]

I think the problem mostly is about how different the concepts are between .NET and JavaScript. All interop async/await have been community contributed and set behind the experimental flag for a reason - I don't have the capacity to debug or ensure the correct working of it and I'm grateful that people like you have time and interest to improve it.

The current async/await in Jint is not fully working either, it would require more work and state handling, it's the same issue as with generators, Jint resolves things too eagerly and there are open issues about it..

[trannamtrung1st]

Got it. I'll close this PR for now and see if I have time on the weekends to draft some ideas. Thanks.

[lofcz]

@trannamtrung1st Thanks for working on this! I've checked the proposed changes and wonder - wouldn't it be reasonable to pull in your work even with the possibility of the continuation running on an unexpected thread? The scenario shown in https://github.com/sebastienros/jint/pull/1882/commits/b0388a1f6bfb0522e666ad2d9dbf7202e5965982 is rather common in real workloads and given the improvement touches just a few files, it seems to me as an interim improvement until/when the event loop is refactored, which appears to be a much bigger undertaking.

The tests would have to be fixed of course.

[trannamtrung1st]

@lofcz I'm happy to reopen the PR if you feel it does help.

[lofcz]

I see the PR as an improvement over the current state, so I'm for reopening it. The tests are however still failing, is that something you could look further into?

[trannamtrung1st]

I see the PR as an improvement over the current state, so I'm for reopening it. The tests are however still failing, is that something you could look further into?

That's the hard part. First, I couldn't replicate it on my Windows 11 PC. Second, as I mentioned before the only possibility I can think of is Continuation being run on a different thread, but I cannot confirm without testing. And if that's the case, I haven't had the solution as well lol. It would be great if anyone can debug the exact cause of test failure on Windows so we can have better solution for it.

[trannamtrung1st]

oh wait, I have an idea but not sure about any side effects. @lahma @lofcz could you guys take a look at the latest commit and trigger the Windows test when you have time?

[trannamtrung1st]

Looks good now. Please let me know if there's any additional step before this one can be merged. Thanks.

[lahma]

Thank you!