client_rust icon indicating copy to clipboard operation
client_rust copied to clipboard

*: Move to `parking_lot` and thus make `owning_ref` obsolete

Open mxinden opened this issue 2 years ago • 10 comments

Before proemtheus-client would use the owning_ref crate to map the target of a std::sync::RwLockReadGuard. owning_ref has multiple unsoundness issues, see https://rustsec.org/advisories/RUSTSEC-2022-0040.html. Instead of replacing owning_ref with a similar crate, we switch to locking via parking_lot which supports the above mapping natively.

Fixes https://github.com/prometheus/client_rust/issues/77

mxinden avatar Aug 06 '22 03:08 mxinden

I wonder whether moving to parking_lot has any implications on this crate's support for WASM.

Gathering the WASM experts here. Do you have any opinions on this? Do we need to pipe through any feature flags?

@PhilippGackstatter, @thomaseizinger, @mriise, @stebalien

mxinden avatar Aug 06 '22 03:08 mxinden

For the record, this patch does not seem to have a significant impact on benchmarks:

➜  critcmp master parking_lot
group                                                  master                                 parking_lot
-----                                                  ------                                 -----------
counter family with Vec<(String, String)> label set    1.00    150.9±1.46ns        ? ?/sec    1.02    153.9±3.00ns        ? ?/sec
counter family with custom type label set              1.00     40.4±0.30ns        ? ?/sec    1.11     44.9±4.67ns        ? ?/sec
encode                                                 1.07     26.5±0.16ms        ? ?/sec    1.00     24.8±1.10ms        ? ?/sec

➜  grep -m 1 'model name' /proc/cpuinfo
model name      : Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz

That said, I don't think the benchmarks are at a point, where I would say that they represent real world use-cases.

mxinden avatar Aug 06 '22 04:08 mxinden

I wonder whether moving to parking_lot has any implications on this crate's support for WASM.

Gathering the WASM experts here. Do you have any opinions on this? Do we need to pipe through any feature flags?

@PhilippGackstatter, @thomaseizinger, @mriise, @Stebalien

Not sure if I'd consider myself a WASM expert :see_no_evil:

Do we currently support compiling to WASM? Looking through the CI config, it doesn't seem to be or at least it is not verified? Adding wasm32-unknown-unknown should at least give us some confidence here although we should bear in mind that often, things compile to wasm but don't actually work at runtime. To be really sure, we would have to add wasm_bindgen_test based tests.

thomaseizinger avatar Aug 06 '22 08:08 thomaseizinger

If WASM support is something we want to guarantee, I think we should make a separate PR that adds the target in the CI cross-compile config, merge that first and see if this PR is affected by it.

thomaseizinger avatar Aug 06 '22 08:08 thomaseizinger

@mxinden and @thomaseizinger , I am a new contributor to this project, I am interested in knowing why we should or shouldn't support WASM ? Why WASM for a client that exports metrics? I am not able to understand the usage of WASM here, can you explain!

AbhijithGanesh avatar Aug 08 '22 10:08 AbhijithGanesh

@mxinden and @thomaseizinger , I am a new contributor to this project, I am interested in knowing why we should or shouldn't support WASM ? Why WASM for a client that exports metrics? I am not able to understand the usage of WASM here, can you explain!

WASM is becoming a more and more relevant deployment target and no longer necessarily means "browser" or "web".

See for example:

  • https://github.com/fermyon/spin
  • https://github.com/lunatic-solutions/lunatic

It is completely feasible that users would want to collect metrics in applications written for either of these two platforms in which case they have to compile the Rust client for prometheus to WASM.

thomaseizinger avatar Aug 08 '22 14:08 thomaseizinger

Note: It looks like parking_lot actually has WASM support. When compiled without (experimental) WASM-atomic support, it just panics when trying to park, but that's fine.

Stebalien avatar Aug 08 '22 21:08 Stebalien

If WASM support is something we want to guarantee, I think we should make a separate PR that adds the target in the CI cross-compile config, merge that first and see if this PR is affected by it.

I've made a PR for that here: https://github.com/prometheus/client_rust/pull/79

thomaseizinger avatar Aug 08 '22 22:08 thomaseizinger

Note: It looks like parking_lot actually has WASM support. When compiled without (experimental) WASM-atomic support, it just panics when trying to park, but that's fine.

Adding to this:

The wasm32-unknown-unknown target is only fully supported on nightly with -C target-feature=+atomics in RUSTFLAGS and -Z build-std passed to cargo. parking_lot will work mostly fine on stable, the only difference is it will panic instead of block forever if you hit a deadlock. Just make sure not to enable -C target-feature=+atomics on stable as that will allow wasm to run with multiple threads which will completely break parking_lot's concurrency guarantees.

https://github.com/Amanieu/parking_lot#nightly-vs-stable

mxinden avatar Aug 10 '22 02:08 mxinden

Unless there are any objections, I will merge and release this tomorrow or next week. Thanks for the input everyone!

mxinden avatar Aug 13 '22 03:08 mxinden