search

elastic / elasticsearch-rs

Official Elasticsearch Rust Client
https://www.elastic.co/guide/en/elasticsearch/client/rust-api/current/index.html
Apache License 2.0
702 stars 72 forks source link

Make ConnectionPool Sync/Send #49

Closed iostat closed 4 years ago

iostat commented 4 years ago

There's currently nothing preventing ConnectionPool from being Sync/Send, but the fact that it's not means I can't use the client in any context other than the most trivial block_on() as seen in the examples. If I want to use the client in any future which is spawned for example (e.g., I have a task which receives on an mpsc::channel, batches up index operations, and makes a bulk call with that batch), the fact that ConnectionPool isn't Sync/Send means the client isn't either, which means my structure which holds the client can't be Sync/Send and thus can't be spawn'd.

Note that you don't even have to have an explicit structure containing the client -- you can't even create a client in an async fn that then .await's the result of a request, as the yield point will force the compiler to generate an intermediate structure which can't be Sync/Send as it holds the client.

This PR simply adds Sync + Send on the supertrait bounds of ConnectionPool, which seems to just work. Not sure if the lack of Sync + Send was a conscious decision or an oversight but hopefully this PR opens a discussion.

Cheers!

russcam commented 4 years ago

Thanks for opening @iostat. I’ll have a chance to properly take a look at this next week when I’m back.

Note that you don't even have to have an explicit structure containing the client -- you can't even create a client in an async fn that then .await's the result of a request, as the yield point will force the compiler to generate an intermediate structure which can't be Sync/Send as it holds the client.

This PR simply adds Sync + Send on the supertrait bounds of ConnectionPool, which seems to just work. Not sure if the lack of Sync + Send was a conscious decision or an oversight but hopefully this PR opens a discussion.

Not a conscious decision :) From initial read, it looks like Transport and ConnectionPool could be `Sync + Send too?

iostat commented 4 years ago

Well the compiler will auto-trait Sync/Send anything whose members are Sync/Send (it’d be bad practice to do it manually anyway as you’d have to unsafe impl to force it)— the only thing that prevented it from auto-traiting Transport was the Box<dyn ConnectionPool>, as nothing about that guarantees Sync/Send. Making ConnectionPool be a supertrait of Sync/Send prevents one from making a ConnectionPool which isn’t Sync + Send— thereby making Transport safe to auto-trait in all cases.