parking_lot_core 0.8.1 wasm link issue

[emilk]

Starting with parking_lot_core 0.8.1, when compiling to wasm there is now a depdendency on "env".

This means one can no longer use parking_lot from within a wasm runtime, such as wasmtime.

When compiling for the web, one can address this problem with parking_lot = { version = "0.11", features = ["wasm-bindgen"] }, but this does not work for non-web wasm runtimes.

Reproduce

Cargo.toml:

[package]
name = "parking_lot_test"
version = "0.1.0"

[dependencies]
parking_lot = "0.11"
parking_lot_core = "=0.8.1"

main.rs:

fn main() {
    let m = parking_lot::Mutex::new(());
    let _lock = m.lock();
}

Using wasm2wat:

§ cargo build --target "wasm32-unknown-unknown"
§ wasm2wat target/wasm32-unknown-unknown/debug/parking_lot_test.wasm | rg import
> (import "env" "now" (func $now (type 7)))

(with parking_lot_core = "=0.8.0" there is no import "env")

[bjorn3]

https://github.com/Amanieu/parking_lot/compare/0.8.0...0.8.1?w=1#diff-f8a0a8b9353ddc2e1efd135d75ec0668f64e2674c15e9b1cda089cbd69f60372 shows no difference in the parking implementation between v0.8.0 and v0.8.1 that could explain this. Does this still happen with LTO enabled?

[emilk]

I tested it now with --release and [profile.release] lto = true – same problem.

[emilk]

I suspect this is related to parking_lot_core:s use of https://github.com/sebcrozet/instant

Does parking_lot require knowing the current time? Because then it is fundamentally incompatible with "pure" wasm

[bjorn3]

The parking_lot algorithm itself need to know the current time for fair locks but the single-threaded wasm option shouldn't need this as there are no other threads against which it needs to be fair.

[emilk]

Interesting!

Is there a way to tell parking_lot that it is compiling for a single-threaded target, so that it does not require the instant crate at all? If not, should there be?

[bjorn3]

It should compile for a single-threaded target by default. Multi-threading requires the bulk memory wasm proposal and atomic instructions. Both of which are not supported for the WASM MVP that is targeted by default. In addition it requires all parts of the program to be compiled with multi-threading support I believe, but libstd isn't by default.

[emilk]

I suspect that the only reason parking_lot 0.8.0 worked and 0.8.1 doesn't is because some subtle failure of rustc to elide unused code (using instant).

---

For my use-case ("pure wasm" = no instant) we would need cfg-blocks around all uses of the instant crate, as well as around functions that uses time, such as wait_until, try_lock_until etc.

For example:

#[cfg(feature = "instant")]
unsafe fn park_until(&self, timeout: Instant) -> bool { ... }

This would be a very intrusive change to the parking_lot code base, disabling all uses of times (Instant and Duration).

(Using #[cfg(not(target_arch = "wasm32"))] would not be a good idea, because when compiling wasm to the web instant works fine with features = ["wasm-bindgen"]).

---

Another alternative is that I create a new wrapper crate around parking_lot which exposes only what I need (Mutex and RwLock) and that has a feature-flag for single-threaded, no-instant compile-targets that just uses a bool to check if a Mutex is already locked (just to panic on reentrant double-lock).

This solution is not very attractive though, as it would require third-party-dependencies to also switch to my new wrapper crate.

Ideally parking_lot would work in a pure wasm (no instant) environment (at least with some special flag)

[faern]

Why do you need locking primitives in a single threaded environment?

[emilk]

Why do you need locking primitives in a single threaded environment?

Great question!

Say you have a library that may or may not be used from a multithreaded context, so the library uses parking_lot::Mutex in a few places. There is also an app that wants to use that library. The app happens to be compiled to a singlethreaded wasm32 target. Should this be possible?

The answer is: it used to be possible, but with parking_lot_core 0.8.1 it no longer is.

So who's responsibility is it to fix this?

The app can do a work-around by addingparking_lot_core = "=0.8.1" to it's Cargo.toml, but that work-around will only work for so long. As soon as the library upgrades to a future `parking_lot_core = 0.9.0" then the app can no use the latest version of that library.

The library could add feature flags around all uses of parking_lot, but there are a lot of libraries using parking_lot, and the app developer will have trouble changing all the libraries.

The third place to fix this is in parking_lot itself, by stating "parking_lot supports pure wasm32 contexts". I think this is the route that is the best.

---

In summary:

Either we say "parking_lot should work for pure wasm32 targets" and work towards that, OR we say "if you use parking_lot in your library, then your library cannot be used by apps that want to compile to pure wasm32 targets".

[faern]

That makes sense. I got the imoression that your code needed it directly and your code was targeting single threaded wasm only :)

[ibaryshnikov]

Isn't it related to this?
rust-lang/rust#72758 (comment)

(import "env" "now" (func $now (type 7)))
it also feels like coming from this line

if let Some(left) = timeout.checked_duration_since(Instant::now()) {

[bjorn3]

That one is only used when wasm atomics are enabled, which aren't by default.

[ibaryshnikov]

Sorry, I misread some of the comments, it feels like the source and the solution have already been found.

[repi]

Would indeed be great to clarify if non-web wasm32-unknown-unknown is intended to not be supported anymore with parking_lot, or if it is and then fix this by not depending on instant and disable the parts of the API that require it timing access.

[Amanieu]

wasm32-unknown-unknown is a bit of weird target: it really should have been a #[no_std] target but instead comes with a mostly non-functional std. parking_lot does require a working std for various features such as Instant (for timed locks/waits), thread-local storage, blocking in the OS, etc.

I'm happy to accept any PRs to support wasm32-unknown-unknown (e.g. by simply disabling all time-related functionality) but in the end this target will have to be supported by users and not by myself (since I don't use WASM personally and am not familiar with all the ways it can be embedded).

[repi]

Yeah wasm32-unknown-unknown is indeed an odd target, would have preferred it to be a lean no-std target (which is how we use it) and have the web bindings using a separate wasm32-web (or similar target), and WASI for std-target for running primarily outside of browser.

For now we haven't run into that many dependencies that we use in wasm32-unknown-unknown that also uses parking_lot, mainly a few of our own that uses parking_lot instead of std as faster primitives for native compilation and for wasm intending to have it just be no-ops in most cases as it is a singlethreaded no-std environment. But so far we've been able to just remove the parking_lot dependencies, that could get harder going forward though but works for now I think.

[fosskers]

This recently popped up again, as https://lib.rs/crates/string_cache just got an update, adding parking_lot as a dependency. string_cache in turn is a dependency of https://lib.rs/crates/scraper .

[dsherret]

My temporary workaround is to add the following to my Cargo.toml file.

[target.'cfg(target_arch = "wasm32")'.dependencies]
parking_lot_core = "=0.8.0"

[d3lm]

Hey folks 👋 I just wanna jump and and provide some context because I am the person that has originally added and also removed the InstantDummy again in favor of instant::Instant (which does support WASM).

While you can lock parking_lot_core to anything below 0.8.1 (uses the InstantDummy) I would not recommend this. Instead I'd recommend to enable the necessary features transitively when using wasm-bindgen. That means for instant to work on wasm32 you have to enable the wasm-bindgen feature and depending on the platform and whether performance.now() is available, you also need to enable inaccurate.

To fix the issue add instant as a dependency to your own crate and enable the features mentioned above:

[features]
# `instant/wasm-bindgen` makes sure it will use `js_sys` for getting the system time
# `instant/inaccurate` uses `Date.now()` instead of `performance.now()`
default = ["instant/wasm-bindgen", "instant/inaccurate"]

[dependencies]
instant = "0.1.12"

Hope this helps.

[repi]

@d3lm That only works for WASM for the web, for running WASM outside of a browser (for example with wasmtime, wasmer, wasm3, wawm etc) as far as I know the only solution is to still unfortunately lock parking lot to before 0.8.1 - which we still have to do.

[dsherret]

@d3lm that unfortunately won’t help me in my scenario because I’m not using the wasm file exclusively in a browser and when I do I’m not using wasm-bindgen. My code is also single threaded… a dependency of a dependency started depending on parking_lot and it broke the wasm interface because parking_lot introduced an import.

I think there should at least be a way for us to manually provide a function to parking_lot that allows it to get the time rather than it adding its own wasm import. That way we can control the wasm interface.

[d3lm]

@repi Oh I see, it's true that this will not work if you run it outside the browser nor with Node.js (target nodejs). I am personally not so familiar with wasmtime as the host, but does it also provide some time function similar to Date.now()? Because then we might want to add a feature to instant instead which doesn't use js_sys if you don't use wasm-bindgen.

[d3lm]

@dsherret Yes I agree, there seems to be an issue that it's not eliminating all the code which should be eliminated because std:time is only used for fairness which would only apply for multi-threading.

[Amanieu]

Does anyone actually use the timeout API on wasm32_unknown_unknown? If not then I think the simplest solution would be to simply cfg-out this API on that target, remove the dependency on instant and just disable fairness for that target.

[repi]

@Amanieu you can't really use it on pure wasm32-unknown-unknown as the Rust std function for Instant::now will panic as it is not implemented.
So just cfging out that functionality when on that platform would definitely sounds like the cleanest and easiest!

[d3lm]

@Amanieu But we can't remove the dependency on instant because std::time panics and if I understand correctly, then it's needed for fairness, no? So we need to have some alternative in place for time related functionality for wasm32-unknown-unknown, but it's quite platform dependent. The web has Date.now() and performance.now() whereas wasmer seems to not have those.

[d3lm]

Oh I see, you mean the timeout here https://github.com/Amanieu/parking_lot/blob/master/core/src/thread_parker/wasm_atomic.rs#L84. We could indeed just use a feature maybe to enable timeouts? And only then it uses instant.

[Amanieu]

I was thinking of just having should_timeout always return false on wasm32-unknown-unknown.

[d3lm]

Yep thats a good idea.

[d3lm]

@Amanieu 🙌