Provider cannot be sent between threads safely

[prabirshrestha]

Trying to add service provider in salvo web framework and seems to fail.

let service_provider = ServiceCollection::default().build_provider();
let router = Router::new()
    .hoop(salvo::affix::inject(service_provider))

48 |         .hoop(salvo::affix::inject(service_provider))
   |               -------------------- ^^^^^^^^^^^^^^^^ `(dyn std::any::Any + 'static)` cannot be sent between threads safely
   |               |
   |               required by a bound introduced by this call

I do have async enabled.

more-di = { version = "1.0.0", features = ["async"] }

[commonsensesoftware]

Interesting that this comes up with Any. Looking at the signature, it looks to require Send + Sync + Clone.

ServiceProvider itself is read-only and it already supports Clone. As far as I can tell, everything is thread-safe under the hood when the async feature is enabled. I think what is missing is that ServiceProvider doesn't have Send + Sync declared when the feature is enabled, which would be an oversight on my part.

Does that sound about right or are you seeing something more?

[prabirshrestha]

Here is the full stack trace.

error[E0277]: `(dyn std::any::Any + 'static)` cannot be sent between threads safely
  --> server/src/server.rs:50:36
   |
50 |         .hoop(salvo::affix::inject(provider))
   |               -------------------- ^^^^^^^^ `(dyn std::any::Any + 'static)` cannot be sent between threads safely
   |               |
   |               required by a bound introduced by this call
   |
   = help: the trait `Send` is not implemented for `(dyn std::any::Any + 'static)`
   = note: required because of the requirements on the impl of `Send` for `Arc<(dyn std::any::Any + 'static)>`
   = note: required because of the requirements on the impl of `Sync` for `spin::once::Once<Arc<(dyn std::any::Any + 'static)>>`
   = note: 1 redundant requirement hidden
   = note: required because of the requirements on the impl of `Sync` for `Arc<spin::once::Once<Arc<(dyn std::any::Any + 'static)>>>`
   = note: required because it appears within the type `ServiceDescriptor`
   = note: required because of the requirements on the impl of `Sync` for `Unique<ServiceDescriptor>`
   = note: required because it appears within the type `alloc::raw_vec::RawVec<ServiceDescriptor>`
   = note: required because it appears within the type `Vec<ServiceDescriptor>`
   = note: required because it appears within the type `(di::Type, Vec<ServiceDescriptor>)`
   = note: required because of the requirements on the impl of `Sync` for `hashbrown::raw::RawTable<(di::Type, Vec<ServiceDescriptor>)>`
   = note: required because it appears within the type `hashbrown::map::HashMap<di::Type, Vec<ServiceDescriptor>, RandomState>`
   = note: required because it appears within the type `HashMap<di::Type, Vec<ServiceDescriptor>>`
   = note: required because of the requirements on the impl of `Send` for `Arc<HashMap<di::Type, Vec<ServiceDescriptor>>>`
   = note: required because it appears within the type `ServiceProvider`
note: required by a bound in `salvo::affix::inject`
  --> /home/prabirshrestha/.cargo/registry/src/github.com-1ecc6299db9ec823/salvo_extra-0.37.1/src/affix.rs:28:18
   |
28 | pub fn inject<V: Send + Sync + Clone + 'static>(value: V) -> AffixList {
   |                  ^^^^ required by this bound in `salvo::affix::inject`

[commonsensesoftware]

Thank you for this. It's going to help. I will look into it further.

[commonsensesoftware]

Apologies for the long over due follow-up and fix. This issue turned about to be easier to fix that I had imagined. The backing data is all read-only and any write operations are all from Clone, which is safe across threads. The only interior mutability scenario is within once used for singletons, but it already has its own locking facilities to make it thread-safe. Long story short, everything was thread-safe, I just need to declare Send and Sync using using the async feature.

I've added a test case which uses a method with your exact method signature, uses a singleton with shared state, and then resolves things across threads. It continues to work as expected. You can see it here:

https://github.com/commonsensesoftware/more-rs-di/blob/0685a3a50bde6dbcc89497c533f94914ff126259/src/di/provider.rs#L434

The container itself cannot guarantee the thread safety of your services nor is it possible to safely provide a way make a service mutable (that I think of - yet). A service that requires mutability must use interior mutability and is also responsible for any synchronization that may be required. If you know the service will always be transient, for example, there's never anything to synchronize.

Thanks again for trying things out and reporting the issue. The patch has been published. If notice something I missed, don't hesitate to re-open the issue or create a new one.