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

dev: future returned by Session::connect() is no longer Send #1065

Closed
gdesmott opened this issue Oct 15, 2022 · 3 comments · Fixed by #1066
Closed

dev: future returned by Session::connect() is no longer Send #1065

gdesmott opened this issue Oct 15, 2022 · 3 comments · Fixed by #1066
Labels

Comments

@gdesmott
Copy link
Contributor

See this simple change to demonstrate the problem:

diff --git a/examples/play.rs b/examples/play.rs
index 1209bf9..9875658 100644
--- a/examples/play.rs
+++ b/examples/play.rs
@@ -12,6 +12,10 @@ use librespot::{
     },
 };
 
+async fn wait_connect(fut: impl std::future::Future<Output = ()> + Send) {
+    fut.await;
+}
+
 #[tokio::main]
 async fn main() {
     let session_config = SessionConfig::default();
@@ -31,10 +35,14 @@ async fn main() {
 
     println!("Connecting...");
     let session = Session::new(session_config, None);
-    if let Err(e) = session.connect(credentials, false).await {
-        println!("Error connecting: {}", e);
-        exit(1);
-    }
+
+    let fut = async {
+        if let Err(e) = session.connect(credentials, false).await {
+            println!("Error connecting: {}", e);
+            exit(1);
+        }
+    };
+    wait_connect(fut).await;
 
     let mut player = Player::new(player_config, session, Box::new(NoOpVolume), move || {
         backend(None, audio_format)

This doesn't work because the Future returned by Session::connect() is not Send.

error: future cannot be sent between threads safely
  --> examples/play.rs:45:5
   |
45 |     wait_connect(fut).await;
   |     ^^^^^^^^^^^^ future created by async block is not `Send`
   |
   = help: the trait `std::marker::Send` is not implemented for `(dyn digest::digest::DynDigest + 'static)`
note: future is not `Send` as this value is used across an await
  --> /var/home/cassidy/dev/rust/librespot/core/src/connection/handshake.rs:95:48
   |
80 |     let padding = rsa::padding::PaddingScheme::new_pkcs1v15_sign(Some(rsa::hash::Hash::SHA1));
   |         ------- has type `rsa::padding::PaddingScheme` which is not `Send`
...
95 |     client_response(&mut connection, challenge).await?;
   |                                                ^^^^^^ await occurs here, with `padding` maybe used later
...
98 | }
   | - `padding` is later dropped here
note: required by a bound in `wait_connect`
  --> examples/play.rs:15:68
   |
15 | async fn wait_connect(fut: impl std::future::Future<Output = ()> + Send) {
   |                                                                    ^^^^ required by this bound in `wait_connect`

error: future cannot be sent between threads safely
  --> examples/play.rs:45:5
   |
45 |     wait_connect(fut).await;
   |     ^^^^^^^^^^^^ future created by async block is not `Send`
   |
   = help: the trait `std::marker::Send` is not implemented for `(dyn rand_core::RngCore + 'static)`
note: future is not `Send` as this value is used across an await
  --> /var/home/cassidy/dev/rust/librespot/core/src/connection/handshake.rs:95:48
   |
80 |     let padding = rsa::padding::PaddingScheme::new_pkcs1v15_sign(Some(rsa::hash::Hash::SHA1));
   |         ------- has type `rsa::padding::PaddingScheme` which is not `Send`
...
95 |     client_response(&mut connection, challenge).await?;
   |                                                ^^^^^^ await occurs here, with `padding` maybe used later
...
98 | }
   | - `padding` is later dropped here
note: required by a bound in `wait_connect`
  --> examples/play.rs:15:68
   |
15 | async fn wait_connect(fut: impl std::future::Future<Output = ()> + Send) {
   |                                                                    ^^^^ required by this bound in `wait_connect`

error: could not compile `librespot` due to 2 previous errors

Because of this librespot cannot longer be used with framework such as Rocket which requires futures to be Send.

This used to work fine with an older version so this is likely a regression.

@gdesmott gdesmott added the bug label Oct 15, 2022
@roderickvd
Copy link
Member

Well regression... this is new functionality but still! Thanks for that minimal working example.

From a quick look it seems that the problem is that one of the variants of rsa::padding::PaddingScheme is not Send but not the one that we are using. I also see that there is an update of that crate with some API changes.

I don't have much time right now but maybe that either of these offer some direction.

@gdesmott
Copy link
Contributor Author

Well regression... this is new functionality but still!

Let's say it's a regression from an older dev version. ;)

From a quick look it seems that the problem is that one of the variants of rsa::padding::PaddingScheme is not Send but not the one that we are using. I also see that there is an update of that crate with some API changes.

PaddingScheme is still !Send in the latest version of the crate so that won't help.

I reported the problem to the rsa crate as well: RustCrypto/RSA#214

gdesmott added a commit to gdesmott/librespot that referenced this issue Oct 20, 2022
rsa::padding::PaddingScheme is !Send, making it impossible to call
Session::connect() with an executor requiring Send futures, such as
Rocket.

Fix librespot-org#1065
@gdesmott
Copy link
Contributor Author

not the one that we are using.

Then we can easily workaround this problem: #1066

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants