chemicstry / wasm_thread

A rust `std::thread` replacement for wasm32 target
Apache License 2.0
123 stars 16 forks source link

Major refactor and cleanup #14

Closed chemicstry closed 5 months ago

chemicstry commented 1 year ago

This is a large overhaul of the library as it has accumutaled a lot of code smell over time.

Major changes:

erwanvivien commented 1 year ago

:warning: So I've tested it on my project & it seems to crash silently? I'll try to track down where it does

erwanvivien commented 1 year ago

Using

wasm_thread = { git = "https://github.com/chemicstry/wasm_thread", rev = "dc0bb62f520ca5af9e3d69e4e8f9a260e36ac777", optional = true, default-features = false }
instead of
wasm_thread = { git = "https://github.com/chemicstry/wasm_thread", rev = "1097fe73fc1775d9ada326dae7021825104293d2", optional = true, default-features = false }

Works, so 1097fe73fc1775d9ada326dae7021825104293d2 introduces some issues

Edit: added my setup https://github.com/erwanvivien/gifski/tree/wasm_thread Everything is started with ./wasm-pack.sh && npx serve -c serve.json pkg

chemicstry commented 1 year ago

⚠️ So I've tested it on my project & it seems to crash silently? I'll try to track down where it does

Is it crashing or just hanging? In later case, could it be that you are using thread::scope incorrectly? Before 1097fe73fc1775d9ada326dae7021825104293d2 it was broken and did not wait for scoped threads to terminate, violating aliasing rules. Now it blocks until all scoped threads exit, just like in std::thread.

erwanvivien commented 1 year ago

It's neither crashing nor hanging, I will heck this tomorrow at work.

But I think you're right, I'm not awaiting scoped threads

erwanvivien commented 1 year ago

I've been trying to check why we don't have anything running when using this branch, but I counldn't find it?

Do you think this test should fail?

#[wasm_bindgen_test]
async fn thread_messaging() {
    use std::{
        sync::mpsc::{channel, Receiver},
        thread as std_thread,
    };

    let (tx, rx) = channel();
    static ATOMIC_COUNT: AtomicUsize = AtomicUsize::new(0);

    fn reader_callback(rx: Receiver<String>) {
        while let Ok(_) = rx.recv() {
            ATOMIC_COUNT.fetch_add(1, Ordering::Relaxed);
            std_thread::sleep(Duration::from_millis(200));
        }
    }

    let reader_thread = thread::Builder::new()
        .name(String::from("reader"))
        .spawn(|| reader_callback(rx))
        .unwrap();

    for i in 0..100 {
        tx.send(format!("message {}", i)).unwrap();
    }

    let _ = thread::spawn(move || {
        std_thread::sleep(Duration::from_millis(1100));

        std::assert_eq!(ATOMIC_COUNT.load(Ordering::Relaxed), 6);
    })
    .join_async()
    .await
    .unwrap();

    reader_thread.join_async().await.unwrap();
}

I get this following stack trace:

     Running tests/wasm.rs (target/wasm32-unknown-unknown/debug/deps/wasm-a2d781d6aa943ef2.wasm)
Set timeout to 20 seconds...
Running headless tests in Chrome on `http://127.0.0.1:63568/`
Try find `webdriver.json` for configure browser's capabilities:
Not found
Failed to detect test as having been run. It might have timed out.
output div contained:
    running 5 tests

driver status: signal: 9 (SIGKILL)                
driver stdout:
    Starting ChromeDriver 112.0.5615.49 (bd2a7bcb881c11e8cfe3078709382934e3916914-refs/branch-heads/5615@{#936}) on port 63568
    Only local connections are allowed.
    Please see https://chromedriver.chromium.org/security-considerations for suggestions on keeping ChromeDriver safe.
    ChromeDriver was started successfully.

Error: some tests failed                          
error: test failed, to rerun pass `--test wasm`

Caused by:
  process didn't exit successfully: `/Users/erwanvivien/.cargo/bin/wasm-bindgen-test-runner /Users/erwanvivien/wasm_thread/target/wasm32-unknown-unknown/debug/deps/wasm-a2d781d6aa943ef2.wasm` (exit status: 1)
Error: Running Wasm tests with wasm-bindgen-test failed
Caused by: failed to execute `cargo test`: exited with exit status: 1
  full command: "cargo" "test" "--target" "wasm32-unknown-unknown" "-Z" "build-std=panic_abort,std"

I'm running the tests with this command:

  RUSTFLAGS='-C target-feature=+atomics,+bulk-memory,+mutable-globals' \
        wasm-pack test --headless --firefox --chrome --chromedriver /Users/erwanvivien/Downloads/chromedriver_mac64/chromedriver -- -Z build-std=panic_abort,std

PS: Sorry for the long delay, we had to try integrate it fully to see if it worked

erwanvivien commented 1 year ago

Ok, I was a bit dumb: this is normal as it wait for the thread at the end for 200ms * 100 => which is exactly the test timeout

chemicstry commented 5 months ago

This has been stuck for a while now and the only issue is the different worker despawn behavior, which can be worked around in user code. I will merge it and release bumping the minor version so it doesn't break anything.