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

Sharing dynamically dispatched registry in web framework state #57

Closed
sd2k opened this issue Mar 25, 2022 · 5 comments · Fixed by #58
Closed

Sharing dynamically dispatched registry in web framework state #57

sd2k opened this issue Mar 25, 2022 · 5 comments · Fixed by #58

Comments

@sd2k
Copy link
Contributor

sd2k commented Mar 25, 2022

I may be missing something but it seems like it's not possible to use the Registry with the default M = SendEncodeMetric type as shared state in web frameworks (e.g. tide::with_state), because those require that the state is all Sync but SendEncodeMetric is not Sync. If I update the tide example to use a dynamically dispatched Registry then it fails to compile:

✦ ❯ cargo c --example tide
error[E0277]: `(dyn SendEncodeMetric + 'static)` cannot be shared between threads safely
   --> examples/tide.rs:25:36
    |
25  |       let mut app = tide::with_state(State {
    |  ___________________----------------_^
    | |                   |
    | |                   required by a bound introduced by this call
26  | |         registry: Arc::new(registry),
27  | |     });
    | |_____^ `(dyn SendEncodeMetric + 'static)` cannot be shared between threads safely
    |
    = help: the trait `Sync` is not implemented for `(dyn SendEncodeMetric + 'static)`
    = note: required because of the requirements on the impl of `Sync` for `Unique<(dyn SendEncodeMetric + 'static)>`
    = note: required because it appears within the type `Box<(dyn SendEncodeMetric + 'static)>`
    = note: required because it appears within the type `(Descriptor, Box<(dyn SendEncodeMetric + 'static)>)`
    = note: required because of the requirements on the impl of `Sync` for `Unique<(Descriptor, Box<(dyn SendEncodeMetric + 'static)>)>`
    = note: required because it appears within the type `alloc::raw_vec::RawVec<(Descriptor, Box<(dyn SendEncodeMetric + 'static)>)>`
    = note: required because it appears within the type `Vec<(Descriptor, Box<(dyn SendEncodeMetric + 'static)>)>`
    = note: required because it appears within the type `Registry`
    = note: required because of the requirements on the impl of `std::marker::Send` for `Arc<Registry>`
note: required because it appears within the type `State`
   --> examples/tide.rs:59:8
    |
59  | struct State {
    |        ^^^^^
note: required by a bound in `with_state`
   --> /Users/ben/.cargo/registry/src/github.com-1ecc6299db9ec823/tide-0.16.0/src/lib.rs:153:20
    |
153 |     State: Clone + Send + Sync + 'static,
    |                    ^^^^ required by this bound in `with_state`

<several related errors omitted>

It's fixed by adding a Sync bound to SendEncodeMetric but it feels like this is a common use case so I imagine it's come up before and I'm doing something silly - am I?

Thanks!

@mxinden
Copy link
Member

mxinden commented Mar 27, 2022

Good catch. Thanks @sd2k. This is related to #55. E.g. when you wrap the Registry in both an Arc and a Mutex, it would compile.

It's fixed by adding a Sync bound to SendEncodeMetric but it feels like this is a common use case so I imagine it's come up before and I'm doing something silly - am I?

I would be OK, extending the super trait SendEncodeMetric to SendSyncEncodeMetric. I expect most folks would run prometheus-client in a multi-threded environment with synchronization on each metric anyways. I still want to give folks the chance to not have to pay in a single threaded setup, though that they can still do overriding the default trait parameter on Registry.

If you agree, want to provide a patch @sd2k?

While wrapping Registry in a Mutex and an Arc (see above) is an option, I would prefer to simpler Arc wrapper only.

sd2k added a commit to sd2k/client_rust that referenced this issue Mar 29, 2022
This will allow the default `Registry` to be used in multi-threaded
environments (e.g. web servers) where the registry needs to be shared
between threads.

Closes prometheus#57.
@sd2k
Copy link
Contributor Author

sd2k commented Mar 29, 2022

Sure @mxinden, sounds good to me. I've created #58 which should hopefully do the trick.

sd2k added a commit to sd2k/client_rust that referenced this issue Mar 29, 2022
This will allow the default `Registry` to be used in multi-threaded
environments (e.g. web servers) where the registry needs to be shared
between threads.

Closes prometheus#57.

Signed-off-by: Ben Sully <ben.sully@grafana.com>
mxinden added a commit that referenced this issue Apr 6, 2022
This will allow the default `Registry` to be used in multi-threaded
environments (e.g. web servers) where a ref to the registry needs
to be shared between threads.

Closes #57.

Signed-off-by: Ben Sully <ben.sully@grafana.com>

Signed-off-by: Max Inden <mail@max-inden.de>

Co-authored-by: Max Inden <mail@max-inden.de>
@Jc2k
Copy link

Jc2k commented May 3, 2022

Any chance of getting a release with this fix in? It's also biting me on a Rocket project.

@mxinden
Copy link
Member

mxinden commented May 3, 2022

@Jc2k see #59

@Jc2k
Copy link

Jc2k commented May 3, 2022

TYVM!

ackintosh pushed a commit to ackintosh/client_rust that referenced this issue Aug 27, 2022
This will allow the default `Registry` to be used in multi-threaded
environments (e.g. web servers) where a ref to the registry needs
to be shared between threads.

Closes prometheus#57.

Signed-off-by: Ben Sully <ben.sully@grafana.com>

Signed-off-by: Max Inden <mail@max-inden.de>

Co-authored-by: Max Inden <mail@max-inden.de>
Signed-off-by: ackintosh <sora.akatsuki@gmail.com>
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 a pull request may close this issue.

3 participants