wasm implementation of RawMutex and RawRwlock using UnsafeCell

[cormacrelf]

This is very rough, but it was borne out of necessity. Essentially, parking is not something you can do in wasm, so I was encountering flurries of parking_lot panics, and not just from #166.

So, this reimplements RawMutex and RawRwLock in the same way std has vastly simplified implementations for wasm. There's a bit of cruft, and I can't claim to understand the meaning of the translated cmpxchg in RwLock but it seems to work, and doesn't panic all the time. Clearly it needs more eyes on it though.

Questions

• Does it actually need the deadlock calls? I think the deadlock system might be using Instant::now.

[bjorn3]

This wont work correctly when using the multi threading wasm extension.

[cormacrelf]

This is true, but neither would the original code without being explicitly designed for the new atomic primitives that aren’t even available on nightly, right? If it needs new code to work I’m not fussed about the exact way it happens to be incorrect now.

[bjorn3]

Maybe add

#[cfg(all(target_arch = "wasm32", target_feature = "atomics"))]
compile_error!("parking_lot doesnt yet support multi threaded wasm");

Also atomic.notify and atomic.wait are exposed on nightly: https://doc.rust-lang.org/stable/core/arch/wasm32/index.html Normal atomics are accessable using the Atomic* types.

[cormacrelf]

I was referring mainly to having to compile libstd from source. Regardless, this is probably a good idea.

[Amanieu]

@alexcrichton You're more familiar with WASM than I am, could you offer your views on this PR?

[alexcrichton]

I don't really know much about the existing primitives and why they won't work "as is" on wasm because there's no contention, but you'll likely want to throw an assert that cfg(target_feature = "threads") is false because these implementations are broken in multithreaded wasm. Otherwise, as seen here, the "implementation for wasm" is basically as it would be for any other platform that only has one thread.

[cormacrelf]

I think this might mostly boil down to more accurate error messages. If you fail to acquire a lock in a single threaded environment, that’s kinda your fault and no amount of waiting can fix it (sorry, me) but the error messages look like parking_lot is missing support. Essentially, this fails faster and helps people understand what their logic errors are. At least, it would if I had done the panic statement copywriting better. The compiled binaries do end up a bit smaller as well.

Is there a cfg flag for any targets that are single threaded?

[Amanieu]

I'm not sure if this is the right approach. My understanding is that only wasm32-unknown-unknown fails to run parking_lot due to the lack of Instant::now. Both wasm32-wasi and wasm32-unknown-emscripten should work fine (I haven't tested this though, so correct me if I'm wrong).

Now wasm32-unknown-unknown is a bit of a special case, because it really should have been a no_std-only target since none of the functions in std work. parking_lot can't really work without OS support, so it doesn't really make sense to support this target. You should probably use a crate like spin-rs instead.

[cormacrelf]

@Amanieu I don't really have a choice, and I suspect a lot of other people don't either. I don't depend on parking_lot directly, I just use my_crate -> salsa -> parking_lot. salsa uses parking_lot for the more exotic features and for performance in, e.g. its flagship use case, rust-analyzer.

Nobody is using mutexes on wasm32-unknown-unknown just for fun. If they actually have control over the targets they support and only need browser wasm, they are using Rc<RefCell<T>> instead. The only people using mutexes are those who have to enable multithreading on full OS targets and also support wasm32 without rewriting the code, i.e. library authors. The standard library implemented a single-threaded Mutex simply because it was easy and would enable a lot of code reuse on wasm. I see no difficulty applying that reasoning to parking_lot, which is similarly widely used.

Using a completely different single-threaded 'mutex' or spin-rs is pointless, because if that worked, you would alread fall into the RefCell bucket. If you don't, then you have two choices: swap between RawMutex implementations inside parking_lot, or ask all downstream crates to write their own crate::Mutex in a cfg_if to swap between the parking_lot::Mutex newtypes and whatever other single-threaded RawMutex implementation, which doesn't actually exist AFAIK, other than this PR. I agree it's not ideal to support this special case in a very OS-dependent crate like parking_lot, but I do think it avoids a bunch of tedious busywork 'supporting wasm' everywhere else.

I think it's appropriate that a single-threaded RawMutex implementation should exist, in any case. Perhaps you would consider putting the RawMutex implementations it in a different crate and swapping them out in parking_lot with a feature flag? Then people who want all their dependencies to use it can just add parking_lot = { ..., features = ["wasm32-unknown-unknown"] } to their own crate. Then parking_lot doesn't have to worry about adapting to wasm multithreading when that comes around, and can focus on doing OS mutexes well.

[Amanieu]

What I'm trying to understand is why you are using wasm32-unknown-unknown in the first place? Is there a reason you can't use wasm32-wasi or wasm32-unknown-emscripten instead?

[Amanieu]

Basically the point that I'm trying to make is that parking_lot requires some sort of OS support, which wasm32-unknown-unknown doesn't have, just like the thumb* targets for microcontrollers.

[cormacrelf]

What I'm trying to understand is why you are using wasm32-unknown-unknown in the first place? Is there a reason you can't use wasm32-wasi or wasm32-unknown-emscripten instead?

@Amanieu Yes. I’m making a library with wasm-pack and wasm-bindgen, to be consumed by JavaScript applications.

What is it about parking_lot that requires OS support? Of course it requires OS support to park threads and interact with a scheduler, but that’s begging the question, since those only exist on OS targets. But I don’t see anything in the API that actually needs it. You have the wait_ methods in Condvar, but if they return immediately, that’s perfectly fine. They don’t actually have to sleep to be correct as described. Synchronous waiting simulates code that returns immediately. If you mean OS support for Instant, yes, that indeed isn’t supported, but single threaded code should be completely ignoring all time-related values anyway, so that other PR was overkill, you can just pass Instant(0).

[Amanieu]

You've convinced me that supporting asm32-unknown-unknown is worthwhile. However I think the approach taken in this PR is the wrong one.

My main concern is that you are duplicating a lot of the existing code, which will make maintenance more difficult in the future. I think that a better approach would be to only change parking_lot_core, which has two uses of Instant::now in FairTimeout. You could replace Instant with a dummy type on wasm32 just for parking_lot.rs and this should resolve your issue.

[d3lm]

Is there any updates on this one? I would really like to use this together with WASM. Would love to see this being merged at some point 👍

[cormacrelf]

@Amanieu is right

[d3lm]

@cormacrelf @Amanieu Does that mean we are not going forward with this at all?

[Amanieu]

My previous suggestion still stands:

I think that a better approach would be to only change parking_lot_core, which has two uses of Instant::now in FairTimeout. You could replace Instant with a dummy type on wasm32 just for parking_lot.rs and this should resolve your issue.

[Amanieu]

I would be happy to accept a PR for it.

[d3lm]

@Amanieu By dummy you mean it does nothing? Won't that break the fair timeout then?

[Amanieu]

Well sure but the lock itself will still work fine. There isn't much else we can do since wasm32-unknown-unknown doesn't give us any timer API we can use.

[d3lm]

True, could we rely on the js_sys::Date maybe?

[Amanieu]

I would prefer just disabling eventual fairness. We don't want to make any assumptions on the environment (WASM can be used outside of browsers).

[d3lm]

Also, would it work for you if we had something like a custom type InstantType and assign that to either a dummy type that provides a now method OR if it's not wasm32 the std::Instant type. And then inside here use InstantType instead.

Detecting the arch with the cfg attribute.

[d3lm]

Right. But that means the RwLock will be somewhat the same as the std::sync::RwLock if it doesn't use some sort of fairness, right?

[Amanieu]

You can still force fairness by unlocking with unlock_fair.

[d3lm]

What do you think about the proposed solution with the cfg attribute and the custom InstantType?

[Amanieu]

That sounds like a good plan. Please keep the changes internal to parking_lot.rs, the public API should keep using the standard Instant type.

[d3lm]

Yep, I would define that custom type inside parking_lot.rs.

[d3lm]

Or what if there was a flag to disable eventual fairness? Would that be something?

[Amanieu]

I wouldn't bother. Just disable it for wasm because it doesn't have a working Instant type.

[d3lm]

Yea makes sense 👍

[d3lm]

Trying to send a PR shortly.

[d3lm]

Really need this 👍

[d3lm]

Here's a PR. Let me know if that works for you or if you want me to change something.

[Pauan]

@Amanieu The way that other libraries (such as rand, chrono, reqwest) handle this is to have a wasm-bindgen Cargo feature, so that way they can have a custom implementation specifically for the web. That's a lot better than using a cfg based on target_arch.