-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add async variants of Atomics.wait #3504
Conversation
Actually, at least according to the docs that I can find, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what happened here, but I would argue that the current wait_bigint
functions are just wrong, they should take a BigInt64Array
instead of a Int32Array
.
But I think we don't need to add to that any further.
Also we could definitely remove the documentation around BigUint64Array
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I was thinking about using this here: wasm-bindgen/crates/futures/src/task/multithread.rs Lines 189 to 190 in 29205c5
But it wouldn't help much because we need the binding to detect support anyway. We also want to have the custom return type to avoid the overhead of |
Yep, I was thinking that/stumbled across that when I was checking whether Regarding the non-async variants of |
Personally I would be fine with it because it looks like a broken API to me, so this is more like fixing a bug then a compatibility issue. But I can't make that call myself. |
I'm fine with such breaking changes. It won't be the first time we made breaking changes to API that no one is/should be using (as an example, removing interface types comes to mind) |
Atomics.waitAsync
now has reasonable support in most browsers with Firefox being the notable exception, however they have a bug report for implementing it.I used the binding for
Atomics.wait
as a reference and one thing that seems off to me is that thetyped_array
argument for the bigint varieties of these functions take anInt32Array
, instead of aBigInt64Array
/BigUint64Array
. It seems to me that the most correct thing to do here is two make further variants of this function, e.g., add the*_biguint
variants, which also take u64 as their value argument. If there is a consensus on this, I can add the variants and we can rename this PR to something like "Clean up Atomics.wait and Atomics.waitAsync".