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

yield_now not completly yields when used with wasm_bindgen #4807

Closed
gzp79 opened this issue Jul 5, 2022 · 5 comments
Closed

yield_now not completly yields when used with wasm_bindgen #4807

gzp79 opened this issue Jul 5, 2022 · 5 comments
Labels
A-tokio Area: The main tokio crate C-bug Category: This is a bug. M-task Module: tokio/task T-wasm Topic: Web Assembly

Comments

@gzp79
Copy link

gzp79 commented Jul 5, 2022

Version
tokio v1.18.0

Platform
wasm target (wasm_bindgen + wasm_bindgen_test)

Description
See the provided code sample.
When tokio yield_now is used, the fetch_with_request never resolves. Both future performs a few steps, the but fetch_with_request never completes when yield_now is used. 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.

I'd expect to reach the unreachable part of the code and as both spawn_local are async and I'd expect to have some progress on both future in some of the next microtask tick.

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 A-tokio Area: The main tokio crate C-bug Category: This is a bug. labels Jul 5, 2022
@Darksonn Darksonn added the M-task Module: tokio/task label Jul 5, 2022
@Darksonn
Copy link
Contributor

Darksonn commented Jul 5, 2022

Considering how yield_now is implemented, this sounds like a bug in your runtime.

@taiki-e
Copy link
Member

taiki-e commented Jul 5, 2022

(If FuturesUnordered is used in your runtime, two yield_now are needed due to this heuristic: rust-lang/futures-rs#2551 (comment))

@gzp79
Copy link
Author

gzp79 commented Jul 6, 2022

Considering how yield_now is implemented, this sounds like a bug in your runtime.

By you who did you address, is it me?
I'm not using any specific runtime. It could be an issue of wasm_bindgen_futures, wasm_bindgen_test or tokio or just simple a limitation of how browsers work but I have no clue how to resolve and I don't have that deep understanding of the executor to resolve it.

It's also interesting that, the spawned future is executed up to the log::trace!("Fetching... {}", i); in both case and only the fetch part gets blocked which is an "imported" feature from the js afaik. The fetch code was taken from the samples of wasm_bindgen doc, no relevant custom code was added.

edit: wasm test async doc

@Darksonn
Copy link
Contributor

Darksonn commented Jul 6, 2022

Yes, by "your runtime" I mean "whatever runtime you are using". And to be clear, you are using a runtime - async code cannot run without one. I assume that the runtime is defined in one of the wasm crates you are using (you aren't using the Tokio runtime).

To give more details regarding what I think is wrong: It sounds like the wasm runtime doesn't check for new IO events if it already has a future that is immediately ready to run. Since your yield_now loop is always ready to do more work, this means that it never sees the new IO events for your spawned task, meaning that the task is unable to make any further progress. Tokio avoids this issue with its event_interval.

@gzp79
Copy link
Author

gzp79 commented Jul 6, 2022

tokio:task::yield_now is not compatible to the runtime used by wasm_bindgen_future (see linked issue).
So I think issue can be closed.

@gzp79 gzp79 closed this as completed Jul 6, 2022
@Darksonn Darksonn added the T-wasm Topic: Web Assembly label Jul 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-bug Category: This is a bug. M-task Module: tokio/task T-wasm Topic: Web Assembly
Projects
None yet
Development

No branches or pull requests

3 participants