Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Callbacks on next tick are not called #16

Closed
ghost opened this issue Aug 4, 2019 · 17 comments · Fixed by #22
Closed

Callbacks on next tick are not called #16

ghost opened this issue Aug 4, 2019 · 17 comments · Fixed by #22
Labels
enhancement New feature or request

Comments

@ghost
Copy link

ghost commented Aug 4, 2019

This code prints the message:

use quick_js::{Context, JsValue};                                                                                       
                                                                                                                        
fn main() {                                                                                                             
    let context = Context::new().unwrap();                                                                              
    context                                                                                                             
        .add_callback("print_cb", |s: String| {                                                              
            println!("value: {}", s);                                                                                   
            JsValue::Null                                                                                               
        })                                                                                                              
        .unwrap();                                                                                                      
    context.eval("print_cb('hi')").unwrap();                                                                            
}                                                                                                                       

However, this code doesn't:

use quick_js::{Context, JsValue};                                                                                       
                                                                                                                        
fn main() {                                                                                                             
    let context = Context::new().unwrap();                                                                              
    context                                                                                                             
        .add_callback("print_cb", |s: String| {                                                              
            println!("value: {}", s);                                                                                   
            JsValue::Null                                                                                               
        })                                                                                                              
        .unwrap();                                                                                                      
    context                                                                                                             
        .eval("new Promise(resolve => resolve('hi')).then(print_cb); null")                                             
        .unwrap();                                                                                                      
}

It is not the case that the program doesn't wait until the callback is called, as adding for example

std::thread::sleep(std::time::Duration::from_millis(1000));                                                         
context.eval("null").unwrap();                                                                                      

at the end of main still doesn't output the message.

@theduke
Copy link
Owner

theduke commented Aug 4, 2019

Yeah I noticed this limitation.

The problem is that the returned value is just a promise and the runtime will have pending jobs.
I haven't seen a way in the C API to drive the event loop until the promise is resolved and then get the resulting value.

@theduke
Copy link
Owner

theduke commented Aug 4, 2019

Probably worth asking for support in the QuickJS mailing list.

@ghost
Copy link
Author

ghost commented Aug 4, 2019

It is implemented in qjs.c somehow, as

new Promise(resolve => resolve('hi')).then(msg => console.log(msg))

works in the console.

I'm trying to understand how is it done there.

@ghost
Copy link
Author

ghost commented Aug 4, 2019

So there is a function js_std_loop, a comment says that it implements the main loop.

An additional method can be added to the Context implementation to invoke that loop, for example after initial evals are made and all callbacks are set.

@theduke
Copy link
Owner

theduke commented Aug 4, 2019

Yeah I saw that function but that doesn't really give us what we need.
Actually, the only way I can see it working right now is with a rather wonky hack:

  • detect that a promise was returned
  • store the promise result in a global variable (by using some helper function in Javascript)
  • run the event loop until the global variable is non-null
  • return the global variable

I'm a bit busy the next few days, but I'd accept a PR.

function __resolvePromise(p) {
  globalThis.__promiseResult = null;
  p
    .then(value => { globalThis.__promiseResult = value; })
    .catch(e => throw e);
}

@theduke theduke added enhancement New feature or request and removed blocked-on-C-api labels Aug 4, 2019
@ghost
Copy link
Author

ghost commented Aug 4, 2019

Yeah I saw that function but that doesn't really give us what we need.

I understood it that js_std_loop, if implemented, can be used this way:

use quick_js::{Context, JsValue};                                                                                       
                                                                                                                        
fn main() {                                                                                                             
    let context = Context::new().unwrap();                                                                              
    context                                                                                                             
        .add_callback("print_cb", |s: String| {                                                              
            println!("value: {}", s);                                                                                   
            JsValue::Null                                                                                               
        })                                                                                                              
        .unwrap();                                                                                                      
    context                                                                                                             
        .eval("new Promise(resolve => resolve('hi')).then(print_cb); null")                                             
        .unwrap();
    context.
        .std_loop()
        .unwrap();
}

The thread would be blocked by std_loop, but std_loop would call the callback and the value would be printed. And in order to run some other Rust code in parallel with the event loop, the context and the event loop should be created in a separate thread and communicate with the main thread for example by passing messages from from the callbacks to the main thread.

Actually, the only way I can see it working right now is with a rather wonky hack:

Yeah, it should work. But as I see it, the loop that would check the global variable probably still might be better to run in a separate thread in order to not do the looping in the main thread.

@theduke
Copy link
Owner

theduke commented Aug 4, 2019

still might be better to run in a separate thread

I wouldn't want to make any threading decisions for the user.

We wouldn't use the js_std_loop function anyway since it is in the libc layer, which is not currently included in the build, and all it does is run JS_ExecutePendingJob in a loop.

Actuall it seems like JS_ExecutePendingJob returns 0 when all work is done, so we could just run until it returns zero and then retrieve the value.

@theduke
Copy link
Owner

theduke commented Aug 4, 2019

As to your point with ctx.loop(): it might actually be cool to add a std::future::Future based interface so you could run multiple tasks concurrently within one runtime.

@ghost
Copy link
Author

ghost commented Aug 4, 2019

Do you mean converting JS promises to Rust futures? So that it could be used like this:

let future1: Future<String> = ctx.eval("new Promise(resolve => resolve('hello 1'))").unwrap();
let future2: Future<String> = ctx.eval("new Promise(resolve => resolve('hello 2'))").unwrap();

and then each call of poll on the resulting future would call JS_ExecutePendingJob (or a higher-level wrapper) until the corresponding promise is resolved or rejected?

@theduke
Copy link
Owner

theduke commented Aug 4, 2019

We would probably need a eval_async() method.

And for this case, we would definitely need a worker thread that runs the loop, keeps track of the futures and resolves them when they are ready.

But the first step for me would be to make the blocking promise handling work.

@theduke
Copy link
Owner

theduke commented Aug 12, 2019

Alright, so basic Promise handling is working now (once the above PR is merged).

If eval or call_function returns a Promise, (detected by duck-typing, as in: .then and .catch must exist), then the event loop is executed until the promise resolves or rejects, and the final value or exception is returned.

Note: currently, setTimeout or setInterval do not exist. Those are in the libc layer of quickjs which is not yet included.

I'll think about how to make this happen.

Example:

    #[test]
    fn eval_async() {
        let c = Context::new().unwrap();

        let value = c
            .eval(
                r#"
            new Promise((resolve, _) => {
                resolve(33);
            })
       "#,
            )
            .unwrap();
        assert_eq!(value, JsValue::Int(33));

        let res = c.eval(
            r#"
            new Promise((_resolve, reject) => {
                reject("Failed...");
            })
       "#,
        );
        assert_eq!(
            res,
            Err(ExecutionError::Exception(JsValue::String(
                "Failed...".into()
            )))
        );
    }

@theduke
Copy link
Owner

theduke commented Aug 12, 2019

@a-rodin you can now basically implement your loop functionality by just running a promise that never resolves, or only resolves once a callback signifies that all work is done, or whatever.

@ghost
Copy link
Author

ghost commented Aug 12, 2019

@theduke I was thinking about it and now I'm a bit concerned about making the loop opaque for the user. The main concern is that it makes it impossible to use in cases when some other event loop is also running, for example the one from tokio. So maybe introducing JsValue::Promise(Future<JsValue>) (where Future can be from futures 0.1 in order to not depend on nightly features) and putting https://github.com/theduke/quickjs-rs/pull/22/files#diff-d07bf61959622437230bf74cba8fd34aR696-R720 to the implementation of poll method of the future could have been more robust?

@theduke
Copy link
Owner

theduke commented Aug 12, 2019

I'd like to keep eval and call_function simple and intuitive. I added warnings about promises to the docs.
I do agree that better futures integration would be nice to have.
But in almost all cases you would want to run anything non-trivial in a dedicated thread, not in one of the tokio threads. They are supposed to block as little as possible.
So we'd probably want a dedicated thread to run the quickjs event loop and return the result of a eval/call_function back to the tokio world with something like futures::channel::oneshot.

@ghost
Copy link
Author

ghost commented Aug 12, 2019

My main use case was to use fetch API from QuickJS. It is not implemented in QuickJS, but can be implemented for example by a third-party crate. The call to fetch mostly consists of waiting for the data, so it looks like a good fit for a tokio thread.

@theduke
Copy link
Owner

theduke commented Aug 12, 2019

I'd actually love to have a fetch api built in as a feature.
I've created two new issues , we can continue to discuss there.
#23 #24

@galvez
Copy link

galvez commented Jul 19, 2020

Using this feature in https://github.com/galvez/fast-vue-ssr/ (now a fully featured Vue.js app up and running!)

I noticed however resolve() (with no value) will cause an error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants