Checking if a returned promise is complete

[ConnerPhillis]

Hi all,

I just started messing around with the new Promise API and had a quick question.

in the following example obj.SimpleTask is a CLR function returning a task, I have written a custom object converter to swap that to an object. The purpose of the function is to wait for a second and then return a random number, simulating IO work.

var result = engine.Evaluate("obj.SimpleTask().then(() => 'done')");
var unwrappedResult = result.UnwrapIfPromise();

When I run the program, I get the following exception:

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

When I add a delay that exceeds the duration of the original wait, as in the following, I am able to extract the result, (in this case 'done') from the promise.

var result = engine.Evaluate("obj.SimpleTask().then(() => 'done')");
await Task.Delay(TimeSpan.FromSeconds(2));
var unwrappedResult = result.UnwrapIfPromise();

So far I haven't been able to find a way to check if the promise that has been extracted from the engine has resolved. When I looked in to UnwrapIfPromise() I saw that there is a promise state object, but with PromiseInstance being an internal class I can't do a type cast to monitor those values.

I'm assuming that I'm probably just missing something, so if any of you have any idea how to check a promise's status let me know.

Thanks in advance!

[lahma]

Converted to discussion and pinging @twop for possible insights and ideas.

[twop]

Hi @ConnerPhillis good question!

I think the mental model of a task does not cleanly map to Promise in this case.

E.g. when you call obj.SimpleTask() I assume you create "ManualPromise" that has explicit resolve/reject methods. And then you add a callback when the task completes and resolve the promise.

Jint doesn't know when the Task completes because it was created in user code.

So before calling UnwrapIfPromise you need to make sure that all manual promises were resolved.

E.g. (pseudocode)

List<Task> pendingTasks;
engine.Set("SimpleTask", () => {
  var task = StartTask();
  var promise = engine.CreateManaulPromise();
  task.ContinueWith(() => promise.Resolve());
  pendingTasks.Add(task); // <------ keep track of all pending
  return promise;
})

var result = engine.Evaluate("obj.SimpleTask().then(() => 'done')");
await Task.All(pendingTasks);// <---- wait until all are finished
// and now it is safe to unwrap because you KNOW that all tasks were completed
var unwrappedResult = result.UnwrapIfPromise();

So conceptually you need to keep track of Tasks you started that map to Promises somewhere outside of Jint.

Hope that helps.

[ConnerPhillis]

Hey @twop,

I'm sorry, I have to disagree

From (what I think is) the latest ECMAScript spec

A Promise is an object that is used as a placeholder for the eventual results of a deferred (and possibly asynchronous) computation.

Also - here's my code to convert the promise, note that I haven't covered tasks with values yet.

public bool TryConvert(Engine engine, object value, out JsValue result)
{
	result = JsValue.Undefined;
	if (value is not Task task)
		return false;
	
	var (promise, resolve, reject) = engine.RegisterPromise();
	Task.Run(async () =>
	{
		try
		{
			await task;
			resolve(JsValue.Undefined);
		}
		catch (Exception e)
		{
			reject(JsValue.FromObject(engine, e));
		}
	});
	result = promise;
	return true;
}

What I have sent back to jint is a promise object that represents a placeholder for the eventual results of a completed task. I am by no means the best at reading and adhering to specifications, so I recognize there could be flaws in my argument, but I personally feel like I've satisfied the mapping.

Also, I want to touch on the task tracking

So conceptually you need to keep track of Tasks you started that map to Promises somewhere outside of Jint.

Let's say that what I want out of Jint's Evaluate function is a JSON object from an external web API and I want to allow the user to script this depending on the data source they're targeting. The JS code may look something like this:

function fetchApiKey() { ... } 
function executeApiRequest(apiKey) { ... }

const requestResultPromise = fetchApiKey().then(executeApiRequest)

return requestResultPromise

In your proposed solution you suggest using Task.WhenAll, The problem here would be that by the time Evaluate is done we would probably still be waiting on the fetchApiKey function. The list of tasks passed to Task.WhenAll would not include the executeApiRequest task yet, however, so Task.WhenAll would complete prematurely.

Additionally, while I can't confirm this without writing test code, when the task yields the second one may not have been added to the list in time, meaning if we checked the list again after WhenAll is done the new task representing encapsulating the next promise may not be present.

I noticed this in the comment for Engine.RegisterPromise, this seems to be what I'm after, but I couldn't find this ExecuteWithEventLoop function. If that exists can someone point me to it?

[twop]

I'm sorry, I have to disagree

Happy to elaborate!

The fundamental difference between Task and Promise is that Promise assumes a single threaded EventLoop while Task has a luxury to have an extra feature of multithreading and be more flexible in terms of runtime (no EventLoop). In other words, you HAVE to think through if and how you need to synchronize Tasks, while Promises are inherently sequential via EventLoop. But I totally agree with you that they have more in common than different, but the devil in the details which are very relevant in the context of JS engine implementation.

In your proposed solution you suggest using Task.WhenAll, The problem here would be that by the time Evaluate is done we would probably still be waiting on the fetchApiKey function. The list of tasks passed to Task.WhenAll would not include the executeApiRequest task yet, however, so Task.WhenAll would complete prematurely.

Additionally, while I can't confirm this without writing test code, when the task yields the second one may not have been added to the list in time, meaning if we checked the list again after WhenAll is done the new task representing encapsulating the next promise may not be present.

Absolutely spot on!

When you run resolve(JsValue.Undefined); it executes JS code that was scheduled with then which in turn can produce more promises. So it actually akin to a recursive problem. Here are the steps

1. Create an empty List of pending Taks/Promises
2. Configure engine with the right built-ins (that add to the list)
3. Run the code with Evaluate
4. Check if the pending list empty
   5.a. If yes you are done!! and it is safe to "Unwrap" the result
   5.b. If not you need to wait for the Pending promises.
   • Copy the list into a temp var waitList
   • clear the current list
   • await Task.WhenAll(waitList)
   • Goto "4"

Note that resolve(JsValue.Undefined) triggers JS code execution and should not be called concurrently. Meaning that it is up to you to ensure "singlethreadness" of JS execution.

Btw that is almost exactly how I use Jint Promises in my project.

I think conceptually you can think about it as of mapping Task + .NET runtime scheduling to JS EventLoop.

Does it make it clearer or even more confusing?

[ConnerPhillis]

Hey, @twop,

Sorry for not getting back to you earlier, but that's quite impressive! As of right now I can't think of anything that I could need that would not be satisfied with the code that you've provided.

@lahma
To your second point - SemaphoreSlim exposes non-async wait methods, maybe Evaluate could be modified to use the same semaphore but calling Wait instead, making Jint thread safe. Not sure of the larger implications of this, just wanted to toss an idea in to the hat.

[lahma]

To your second point - SemaphoreSlim exposes non-async wait methods, maybe Evaluate could be modified to use the same semaphore but calling Wait instead, making Jint thread safe. Not sure of the larger implications of this, just wanted to toss an idea in to the hat.

Problem with that is that I've seen people calling engine.Evalute inside the own c# code running inside the engine so I would presume that kind code would deadlock as it's a nested call. This is still thread-safe as we expect end user to call single-threadedly and thus no need for locks.

[twop]

Great work! Seems quite simple, yet powerful.

^_^

Some notes about tighter integration (out-of-the-box experience) based on my limited understanding: if AllowClr is true, we could just add the Task converter
EvaluateAsync could do something like if(_asyncContext is not null) throw new InvalidOperationException("concurrent calls not allowed") or similar mechanism to block, so initializing and disposing the context per EvaluateAsync call?

Unfortunately, the way the code is structured you cannot do it right now. Converter modifies state of AsyncContext, meaning that context has to be created before converter, thus there is a need for this awkward PrepareWithAsyncContext. Note that this pain would go away if you can add and remove converters after Engine creation. Then the entire logic can be moved into just EvaluateAsync.

the semaphore might get confusing as engine is not thread safe and then this single operation would be

Semaphore is needed to make sure Tasks report their results in a queue vs concurrently. Or maybe I misunderstood your question? Note that thread safety of EvaluateAsync is just a side benefit of that, we can just omit that from the docs/notes altogether, people should not call EvaluateAsync concurrently.

Would it make sense to expose public API as ValueTask?

Totally, I just modified the original example which used Tasks.

This is quite integral to interop story so I'd think it would be nice to have it as part of the normal distribution with remark of unstable API or something similar.

If it is possible I would prefer it to be an add-on. Ideally even something like a Jint optional package JintTask2Promise.

@ConnerPhillis I would try to avoid using any threadsafety mechanisms if they are not needed. They incur performance and maintenance costs. Before .NET6 tree-shaking is a concern, shipping code that you don't need is not great on constrained environments.

[lahma]

Some notes about tighter integration (out-of-the-box experience) based on my limited understanding: if AllowClr is true, we could just add the Task converter
EvaluateAsync could do something like if(_asyncContext is not null) throw new InvalidOperationException("concurrent calls not allowed") or similar mechanism to block, so initializing and disposing the context per EvaluateAsync call?

Unfortunately, the way the code is structured you cannot do it right now. Converter modifies state of AsyncContext, meaning that context has to be created before converter, thus there is a need for this awkward PrepareWithAsyncContext. Note that this pain would go away if you can add and remove converters after Engine creation. Then the entire logic can be moved into just EvaluateAsync.

I was thinking that AsyncContext could have been a field for Engine and engine initialization (where options are applied) could check if AllowClr and then instantiate the async context and hook the converter (I didn't see the dependency converter has before there, but workable). So engine could instantiate at construction and configure as required, there's all the possibilities when Engine knows about the feature.

the semaphore might get confusing as engine is not thread safe and then this single operation would be

Semaphore is needed to make sure Tasks report their results in a queue vs concurrently. Or maybe I misunderstood your question? Note that thread safety of EvaluateAsync is just a side benefit of that, we can just omit that from the docs/notes altogether, people should not call EvaluateAsync concurrently.

I understood the semaphore more like being engine lock, but it's context lock, OK.

Would it make sense to expose public API as ValueTask?

Totally, I just modified the original example which used Tasks.

👍🏻

This is quite integral to interop story so I'd think it would be nice to have it as part of the normal distribution with remark of unstable API or something similar.

If it is possible I would prefer it to be an add-on. Ideally even something like a Jint optional package JintTask2Promise.

I still feel that as this is not in it's current form dependent of libraries or special constructs, and being quite small, it could fit the engine as service too.

[twop]

I was thinking that AsyncContext could have been a field for Engine and engine initialization (where options are applied) could check if AllowClr and then instantiate the async context and hook the converter (I didn't see the dependency converter has before there, but workable). So engine could instantiate at construction and configure as required, there's all the possibilities when Engine knows about the feature.

That is a good point about the engine knowing about AsyncContext. I think if this is a built-in feature then maybe dedicate a flag for this. Or have some other 1st class API support for this use case.

Regardless I would love if the Task interop is purely opt-in, and promises can work without it (like I use them in my project).

I don't feel that I have enough stake in the subject to make a call about API surface area, but I would love to be a part of the brainstorming process.

[twop]

I noticed this in the comment for Engine.RegisterPromise, this seems to be what I'm after, but I couldn't find this ExecuteWithEventLoop function. If that exists can someone point me to it?

Good find! It used to be two functions Execute ExecuteWithEventLoop but now Execute combines both, thus the comment is outdated :( Sorry for confusion

[ConnerPhillis]

No problem, that makes perfect sense!