Add initial support for promises #1923
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1923 +/- ##
==========================================
- Coverage 45.98% 45.90% -0.08%
==========================================
Files 206 209 +3
Lines 17057 17317 +260
==========================================
+ Hits 7844 7950 +106
- Misses 9213 9367 +154
Continue to review full report at Codecov.
|
Razican
added
builtins
Razican
requested review from
Razican,
jedel1043,
raskad,
HalidOdat,
jasonwilliams and
RageKnify
VM implementation
Fixed tests (126):New panics (2): |
|
I really like how this looks! It's a great step in the right direction. Maybe you can already remove the ignored flag:async tests from the test_ignore.txtI still have to review this properly (even though most of those require extra tester support). In any case, there is something that has already caught my attention. The usage of the queues crate. This crate was last updated in 2018 (more than 3 years ago), so I'm not comfortable adding it as a dependency. In fact, I might even prefer to create a "boa_queues" crate or something and adapt it. The crate itself is only 1 Rust file (we need to check licensing, though). |
| struct ResolvedRecord { | ||
| value: bool, | ||
| } | ||
|
|
||
| struct ResolvingFunctionsRecord { | ||
| resolve: JsValue, | ||
| reject: JsValue, | ||
| } | ||
|
|
||
| #[derive(Debug, Trace, Finalize)] | ||
| struct RejectResolveCaptures { | ||
| promise: JsObject, | ||
| already_resolved: JsObject, | ||
| } |
There was a problem hiding this comment.
Could we add some documentation to these structures? Also, they must all implement Debug, Clone and Copy, when possible
| #[derive(Debug, Clone, Trace, Finalize)] | ||
| enum PromiseState { | ||
| Pending, | ||
| Fulfilled, | ||
| Rejected, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, Trace, Finalize)] | ||
| pub struct Promise { | ||
| promise_result: Option<JsValue>, | ||
| promise_state: PromiseState, | ||
| promise_fulfill_reactions: Vec<ReactionRecord>, | ||
| promise_reject_reactions: Vec<ReactionRecord>, | ||
| promise_is_handled: bool, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, Trace, Finalize)] | ||
| pub struct ReactionRecord { | ||
| promise_capability: Option<PromiseCapability>, | ||
| reaction_type: ReactionType, | ||
| handler: Option<JobCallback>, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, Trace, Finalize)] | ||
| enum ReactionType { | ||
| Fulfill, | ||
| Reject, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, Trace, Finalize)] | ||
| struct PromiseCapability { | ||
| promise: JsValue, | ||
| resolve: JsValue, | ||
| reject: JsValue, | ||
| } | ||
|
|
||
| #[derive(Debug, Trace, Finalize)] | ||
| struct PromiseCapabilityCaptures { | ||
| promise_capability: Gc<boa_gc::Cell<PromiseCapability>>, | ||
| } |
There was a problem hiding this comment.
I would like to have some documentation for these structures, to understand their purpose, if there is any reference to them in the spec, and ReactionType and PromiseState can probably implement Copy, which would make things easier.
For Copy types, you will probably need to implement Trace and Finalize separately, with an empty implementation, or better: whenever a structure requires them in, use the #[unsafe_ignore_trace] attribute on the field.
| @@ -0,0 +1,731 @@ | |||
| //! This module implements the global `Promise` object. | |||
|
|
|||
| #![allow(dead_code, unused_results, unused_variables)] | |||
There was a problem hiding this comment.
I think this shouldn't stay here, right? We don't want to silence warnings :)
There was a problem hiding this comment.
You're absolutely right, forgot to remove this :)
|
|
||
| use super::{Promise, PromiseCapability}; | ||
|
|
||
| pub(crate) struct PromiseJob; |
There was a problem hiding this comment.
Could we add some documentation here, add the reference to the spec, and derive Debug, Clone and Copy for it?
| #[derive(Debug, Trace, Finalize)] | ||
| struct ReactionJobCaptures { | ||
| reaction: ReactionRecord, | ||
| argument: JsValue, | ||
| } |
There was a problem hiding this comment.
This would need some documentation to explain where does it come from, what it's used for, how...
| #[derive(Debug, Trace, Finalize)] | ||
| struct JobCapture { | ||
| promise_to_resolve: JsObject, | ||
| thenable: JsValue, | ||
| then: JobCallback, | ||
| } | ||
|
|
||
| impl JobCapture { | ||
| fn new(promise_to_resolve: JsObject, thenable: JsValue, then: JobCallback) -> Self { | ||
| Self { | ||
| promise_to_resolve, | ||
| thenable, | ||
| then, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This would need some documentation to know where it's coming from, how to use it, what it means and so on. Maybe even a spec link. Also for the new() function.
| #[derive(Debug, Clone, Trace, Finalize)] | ||
| pub struct JobCallback { |
There was a problem hiding this comment.
I would like to have a bit of documentation in this structure, to understand it better.
| pub fn make_job_callback(callback: JsValue) -> Self { | ||
| Self { | ||
| callback: Box::new(callback), | ||
| } | ||
| } | ||
|
|
||
| pub fn call_job_callback( | ||
| &self, | ||
| v: &JsValue, | ||
| argument_list: &[JsValue], | ||
| context: &mut Context, | ||
| ) -> JsResult<JsValue> { | ||
| let callback = match *self.callback { | ||
| JsValue::Object(ref object) if object.is_callable() => object.clone(), | ||
| _ => panic!("Callback is not a callable object"), | ||
| }; | ||
|
|
||
| callback.__call__(v, argument_list, context) | ||
| } | ||
|
|
||
| pub fn run(&self, context: &mut Context) { | ||
| let callback = match *self.callback { | ||
| JsValue::Object(ref object) if object.is_callable() => object.clone(), | ||
| _ => panic!("Callback is not a callable object"), | ||
| }; | ||
|
|
||
| let _callback_result = callback.__call__(&JsValue::Undefined, &[], context); | ||
| } |
There was a problem hiding this comment.
Could we add documentation to these functions?
|
Does this help us get toward allowing https://boa-dev.github.io/boa/doc/boa_engine/builtins/function/type.NativeFunctionSignature.html to be asynchronous? |
This contribution does not add an executor/async rust code, all code that you would add as native functions will be executed on the single/main rust thread that executes all JS code in order of the jobqueue after executing the main program (run-to-completion). Say that you would now add I think it helps towards async builtin functions as it expands upon the ECMAScript standards but that would require making the VM async. |
|
Even if it is still single-threaded run-to-completion, could we change the NativeFunctionSignature to allow a function that returns a Future? Even if it's blocking under-the-hood, would it still work? So for example imagine creating a custom Rust function (that does an http request or something) that returns a Future exposed to boa as a function that returns a Promise in JS. Even if it's technically synchronous, this could be very useful for my use case. |
We should manage expectations here. It's important to understand that ECMAScript is single-threaded. Even its async/await is single-threaded always. This requires some guarantees (such as types not implementing So, What is sure is that while we don't have asynchronous JavaScript, we cannot have asynchronous Rust. And even if/when implement asynchronous Rust, synchronous JavaScript and Rust must still be supported. So, this is a huge step towards enabling asynchronous JavaScript to run in Boa, and this could potentially open the doors to use asynchronous Rust native functions at some point, but it will be very, very tricky. For now, you can use |
|
Thank you very much for the in-depth explanation. Unfortunately I am working in a constrained Wasm environment where block_on does not work, hence my great desire for asynchronus JS and Rust (since it blocks so many capabilities I am trying to give my users). Thanks! |
|
Hopefully i can take a proper look at this this week |
|
@aaronmunsters did you have the chance to look at the review comments? |
|
Hi there, unfortunately not yet. This week I've got another deadline coming up, but I'll get at it starting next week 😊 |
There was a problem hiding this comment.
Looks like a great start to me. I agree it will need some tidying up from the comments. Job itself will need some documentation describing what it is and how it's used etc etc.
I like the test showing the value after completion.
Having something like mio handle our queue in future for tasks would be nice.
I think V8 doesn't actually deal with any of that, and Node just adds that logic on, i wonder if we'll eventually need a flag like we had with console.
Long term thinking but I wonder if we'll need to make a boa_runtime which uses boa_engine as a dependency.
| @@ -709,10 +714,21 @@ impl Context { | |||
| self.realm.set_global_binding_number(); | |||
| let result = self.run(); | |||
| self.vm.pop_frame(); | |||
| self.run_queued_jobs(); | |||
There was a problem hiding this comment.
one thing that confused me, is in your example i would have expected count to be three. Because execute waits here for the promise job to finish before finishing. But it seems that's not the case. It looks like it returns 2 and you need to call it again (looking at your test).
Can someone explain that?
There was a problem hiding this comment.
The Promise constructor executes its code on creation, and then is queued on the job queue. After executing all the statements of the script, count === 2 and then is executed, making count === 3. However, the script already returned 2 as the evaluation result, since let result = self.run() executes before self.run_queued_jobs(). When the next execution occurs, count is already 3, so the evaluation returns 3.
That's not entirely true. The async rust model doesn't require that types implement |
|
Whether there is progress in this feature, it is a very practical feature, I am looking forward to its completion. |
| } | ||
|
|
||
| impl JobCallback { | ||
| pub fn make_job_callback(callback: JsValue) -> Self { |
There was a problem hiding this comment.
We should verify our invariants on creation, like checking that the input is a callable JsObject (which complements the other comment about storing a JsObject). This would allow us to skip the checks on call_job_callback and run.
| @@ -0,0 +1,39 @@ | |||
| use crate::{Context, JsResult, JsValue}; | |||
There was a problem hiding this comment.
Maybe this should be moved to the promise module, but I'll like to hear your rationale about putting it on the root, since I'm still not sure.
| #[derive(Debug, Trace, Finalize)] | ||
| struct RejectResolveCaptures { | ||
| promise: JsObject, | ||
| already_resolved: JsObject, | ||
| } |
There was a problem hiding this comment.
You can move this inside create_resolving_functions. This should limit its scope, since it's only used internally, and it would make it renameable to only Captures. Also applies to every ...Captures struct.
There was a problem hiding this comment.
This is also being used in job.rs, so I left it as-is in #2107. Feel free to review it there.
|
@aaronmunsters did you have the chance to look at this? |
|
Closing in favour of #2107. |
This PR overrides #1923. It also removes the `queues` dependency added there, and rebases it to the latest `main` branch state. It adds the following: - A job queue (in `Context`) - The constructor [`Promise`](https://tc39.es/ecma262/#sec-promise-executor) - [`Promise.race`](https://tc39.es/ecma262/#sec-promise.race) - [`Promise.reject`](https://tc39.es/ecma262/#sec-promise.reject) - [`Promise.resolve`](https://tc39.es/ecma262/#sec-promise.resolve) - [`get Promise [ @@species ]`](https://tc39.es/ecma262/#sec-get-promise-@@species) - [`Promise.prototype [ @@toStringTag ]`](https://tc39.es/ecma262/#sec-promise.prototype-@@tostringtag) - [`Promise.prototype.then`](https://tc39.es/ecma262/#sec-promise.prototype.then) - [`Promise.prototype.finally`](https://tc39.es/ecma262/#sec-promise.prototype.finally) - [`Promise.prototype.catch`](https://tc39.es/ecma262/#sec-promise.prototype.catch) - The additional needed infrastructure - [`PerformPromiseThen ( promise, onFulfilled, onRejected [ , resultCapability ] )`](https://tc39.es/ecma262/#sec-performpromisethen) - [`TriggerPromiseReactions ( reactions, argument )`](https://tc39.es/ecma262/#sec-triggerpromisereactions) - [`PerformPromiseRace ( iteratorRecord, constructor, resultCapability, promiseResolve )`](https://tc39.es/ecma262/#sec-performpromiserace) - [`RejectPromise ( promise, reason )`](https://tc39.es/ecma262/#sec-rejectpromise) - [`FulfillPromise ( promise, value )`](https://tc39.es/ecma262/#sec-fulfillpromise) - [`IfAbruptRejectPromise ( value, capability )`](https://tc39.es/ecma262/#sec-ifabruptrejectpromise) - [`CreateResolvingFunctions ( promise )`](https://tc39.es/ecma262/#sec-createresolvingfunctions) - [`NewPromiseCapability ( C )`](https://tc39.es/ecma262/#sec-newpromisecapability) - [`NewPromiseReactionJob ( reaction, argument )`](https://tc39.es/ecma262/#sec-newpromisereactionjob) - [`NewPromiseResolveThenableJob ( promiseToResolve, thenable, then )`](https://tc39.es/ecma262/#sec-newpromiseresolvethenablejob) - [`PromiseResolve ( C, x )`](https://tc39.es/ecma262/#sec-promise-resolve) - A test case showcasing the run-to-completion semantics. An example program that shows the control flow with this addition is: ```javascript new Promise((res, rej) => { console.log("A"); res(undefined); }).then((_) => console.log("B")); console.log("C"); ``` Which would output: ``` A C B ```
It adds the following:
Context)PromisePromise.prototype.thenPerformPromiseThen ( promise, onFulfilled, onRejected [ , resultCapability ] )TriggerPromiseReactions ( reactions, argument )RejectPromise ( promise, reason )FulfillPromise ( promise, value )CreateResolvingFunctions ( promise )NewPromiseCapability ( C )NewPromiseReactionJob ( reaction, argument )NewPromiseResolveThenableJob ( promiseToResolve, thenable, then )An example program that shows the control flow with this addition is:
Which would output:
Feedback is most welcome! 😉