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

Major refactor and cleanup #14

Merged
merged 24 commits into from
Mar 17, 2024
Merged

Major refactor and cleanup #14

merged 24 commits into from
Mar 17, 2024

Conversation

chemicstry
Copy link
Owner

@chemicstry chemicstry commented Mar 10, 2023

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

Major changes:

  • Removed async-channel dependency and implemented proper result exchange via Packet like in std lib. This should now allow async joins in the main thread where previously they would panic.
  • Fixed multiple soundness bugs with mutable static access.
  • Replaced main thread mutexes that could panic with spin locks.
  • Implemented helper utilities available_parallelism() and is_web_worker_thread().
  • Added better formatting configuration.
  • Added wasm-pack tests.

@@ -13,19 +13,34 @@ categories = ["concurrency", "wasm"]
readme = "README.md"

[features]
default = ["es_modules"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only problem I had with the PR, I needed to update { default-features = false }

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought module support is now common enough to be default. I will bump the version of course.

@erwanvivien
Copy link
Contributor

erwanvivien commented Mar 14, 2023

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

@erwanvivien
Copy link
Contributor

erwanvivien commented Mar 14, 2023

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 1097fe7 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
Copy link
Owner Author

chemicstry commented Mar 14, 2023

⚠️ 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 1097fe7 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
Copy link
Contributor

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
Copy link
Contributor

erwanvivien commented Apr 7, 2023

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
Copy link
Contributor

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
Copy link
Owner Author

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.

@chemicstry chemicstry merged commit cd6487e into main Mar 17, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants