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

tokio::task::yield_now() is not completely yields and fetch never completes #2979

Closed
gzp79 opened this issue Jul 6, 2022 · 11 comments
Closed
Labels

Comments

@gzp79
Copy link

gzp79 commented Jul 6, 2022

Description
See the provided code sample.
When tokio yield_now is used, the fetch_with_request never resolves. Both loop performs a few steps but fetch_with_request never completes when yield_now is used (The Fetching... is logged, but unreachable is never reached). If I replace it with wasm_timer::Delay with a 0 Duration, the fetch works as expected and the download completes with either success or error.

The error was originally reported in for tokio but it seems as this is the correct place to report/ask.

use wasm_bindgen_futures::JsFuture;
use web_sys::{Request, RequestInit, RequestMode};
use wasm_bindgen_test::wasm_bindgen_test as test;

#[test]
async fn yield_bug() {
    wasm_logger::init(::wasm_logger::Config::new(log::Level::Trace));
        
    wasm_bindgen_futures::spawn_local(async move {
        let mut i = 0;
        loop {
            let mut opts = RequestInit::new();
            opts.method("GET");
            opts.mode(RequestMode::Cors);
            let url = "https://example.com";
            let request = Request::new_with_str_and_init(&url, &opts).unwrap();

            let window = web_sys::window().unwrap();
            log::trace!("Fetching... {}", i);
            match JsFuture::from(window.fetch_with_request(&request)).await {
                Ok(_) => log::trace!("OK {}", i),
                Err(_) => log::trace!("ERROR {}", i),
            };            
            i += 1;

            unreachable!("For some reason we never get here with tokio::task::yield_now()");
        }
    });

    loop {
        let mut i = 0;
        loop {
            if i % 100000 == 0 {
                log::trace!("CC {}", i);
            }
            tokio::task::yield_now().await;
            //let _ = wasm_timer::Delay::new(std::time::Duration::from_micros(0)).await;
            i += 1;
        }
    }
}
@gzp79 gzp79 added the bug label Jul 6, 2022
@hamza1311 hamza1311 added question and removed bug labels Jul 6, 2022
@hamza1311
Copy link
Collaborator

tokio::task::yield_now's documentation says:

Yields execution back to the Tokio runtime.

Your is not running in a Tokio runtime. wasm_bindgen_futures::spawn_local is effectively a wrapper around new Promise(...). You can't use Tokio runtime specific function while running a JS promise

@gzp79
Copy link
Author

gzp79 commented Jul 6, 2022

Thanks. In that case is there some similar "yield" feature ? wasm_timer could be an option, but that crate seems to be not maintained any more and the latest release has some issue: tomaka/wasm-timer#14

@acheroncrypto
Copy link

fluvio-wasm-timer works for this.

@gzp79
Copy link
Author

gzp79 commented Jul 6, 2022

fluvio-wasm-timer works for this.

Yes, this is what I've also linked (one of the last comment in the issue), but as its documentation states it is just a temporal solution which is not too convincing and does not seems to be a stable solution:
This branch is just a work around to fix [a wasm bug](https://github.com/tomaka/wasm-timer/pull/13).

@hamza1311
Copy link
Collaborator

Does gloo-timers solve the problem for you?

@gzp79
Copy link
Author

gzp79 commented Jul 6, 2022

Does gloo-timers solve the problem for you?

hm, seems to be backed up by a working group for wasm but never heard of it before. Thank I will give it a try.

@hamza1311
Copy link
Collaborator

hm, seems to be backed up by a working group for wasm but never heard of it before

It's in the same organization as this repository, though I'm currently the only one maintaining it. If you encounter any issues, please file them at https://github.com/rustwasm/gloo

@Darksonn
Copy link

Darksonn commented Jul 6, 2022

Please be aware that yield_now is completely runtime independent. The documentation mentions Tokio, but it should work in any runtime.

@hamza1311
Copy link
Collaborator

hamza1311 commented Jul 7, 2022

@Darksonn would you consider this (completely single threaded) (or this (uses atomics)) to be a "runtime" where yield_now works? If so, what should it actually be doing?

Note that spawn_local calls Task::spawn

@gzp79
Copy link
Author

gzp79 commented Jul 8, 2022

From my point of view I've managed to solve my issue with some refactor that don't require yield any more (and that is a much preferred solution). Thanks for the valuable comments on all forums/threads, now I have a bit better understanding of the system.

If you feel as the wasm bindgen runtime can be improved anyway w.r.t the original question, than please keep this ticket open, but from my side I'd suggest closing it.

@Darksonn
Copy link

Darksonn commented Jul 9, 2022

@hamza1311 Whether yield_now results in starvation normally depends on the loop that calls run rather than the run method itself. Your run method looks fine to me.

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

No branches or pull requests

4 participants