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

A problem with switching from 0.3 to 0.4 #370

Closed
importcjj opened this issue Dec 17, 2019 · 9 comments
Closed

A problem with switching from 0.3 to 0.4 #370

importcjj opened this issue Dec 17, 2019 · 9 comments

Comments

@importcjj
Copy link

importcjj commented Dec 17, 2019

I use mobc as my connection pool.
Here is the HelloWorld demo.

pub struct Global {
    redis: Pool<RedisConnectionManager<DefaultExecutor>>,
    postgres: Pool<PostgresConnectionManager<NoTls, DefaultExecutor>>,
}

async fn helloworld(cx: Request<Global>) -> String { 
    match cx.state().redis.get().await {
        Ok(_) => "Rust rua!!!".to_string(),
        Err(_e) => "SERVER ERROR".to_string(),
    }
}

It's ok in tide 0.3, but it complied failed in tide 0.4

Here is the error message, my pool is sync + send + 'static, I don't know which part is unsync

error[E0277]: `(dyn futures_io::if_std::AsyncBufRead + std::marker::Send + 'static)` cannot be shared between threads safely
  --> api/src/main.rs:84:17
   |
84 |     app.at("/").get(helloworld);
   |                 ^^^ `(dyn futures_io::if_std::AsyncBufRead + std::marker::Send + 'static)` cannot be shared between threads safely
   |
   = help: the trait `std::marker::Sync` is not implemented for `(dyn futures_io::if_std::AsyncBufRead + std::marker::Send + 'static)`
   = note: required because of the requirements on the impl of `std::marker::Sync` for `std::ptr::Unique<(dyn futures_io::if_std::AsyncBufRead + std::marker::Send + 'static)>`
   = note: required because it appears within the type `std::boxed::Box<(dyn futures_io::if_std::AsyncBufRead + std::marker::Send + 'static)>`
   = note: required because it appears within the type `std::pin::Pin<std::boxed::Box<(dyn futures_io::if_std::AsyncBufRead + std::marker::Send + 'static)>>`
   = note: required because it appears within the type `http_service::Body`
   = note: required because it appears within the type `http::request::Request<http_service::Body>`
   = note: required because it appears within the type `tide::request::Request<Global>`
   = note: required because of the requirements on the impl of `std::marker::Send` for `&tide::request::Request<Global>`
   = note: required because it appears within the type `for<'r, 's, 't0, 't1> {tide::request::Request<Global>, &'r tide::request::Request<Global>, Global, &'s Global, &'t0 mobc::Pool<mobc_postgres::PostgresConnectionManager<tokio_postgres::tls::NoTls, mobc::runtime::DefaultExecutor>>, mobc::Pool<mobc_postgres::PostgresConnectionManager<tokio_postgres::tls::NoTls, mobc::runtime::DefaultExecutor>>, impl std::future::Future, ()}`
   = note: required because it appears within the type `[static generator@api/src/main.rs:34:52: 43:2 cx:tide::request::Request<Global> for<'r, 's, 't0, 't1> {tide::request::Request<Global>, &'r tide::request::Request<Global>, Global, &'s Global, &'t0 mobc::Pool<mobc_postgres::PostgresConnectionManager<tokio_postgres::tls::NoTls, mobc::runtime::DefaultExecutor>>, mobc::Pool<mobc_postgres::PostgresConnectionManager<tokio_postgres::tls::NoTls, mobc::runtime::DefaultExecutor>>, impl std::future::Future, ()}]`
   = note: required because it appears within the type `std::future::GenFuture<[static generator@api/src/main.rs:34:52: 43:2 cx:tide::request::Request<Global> for<'r, 's, 't0, 't1> {tide::request::Request<Global>, &'r tide::request::Request<Global>, Global, &'s Global, &'t0 mobc::Pool<mobc_postgres::PostgresConnectionManager<tokio_postgres::tls::NoTls, mobc::runtime::DefaultExecutor>>, mobc::Pool<mobc_postgres::PostgresConnectionManager<tokio_postgres::tls::NoTls, mobc::runtime::DefaultExecutor>>, impl std::future::Future, ()}]>`
   = note: required because it appears within the type `impl std::future::Future`
   = note: required because it appears within the type `impl std::future::Future`
   = note: required because of the requirements on the impl of `tide::endpoint::Endpoint<Global>` for `fn(tide::request::Request<Global>) -> impl std::future::Future {helloworld}`
@yoshuawuyts
Copy link
Member

@importcjj o/ thanks for reporting! At a glance I'm unsure what's causing this failure. Just to verify: which Rust version are you using? Did you only update the Tide version, or also Rust versions?

I just wrote a quick example to verify that we can indeed read from the global state:

use async_std::io;
use async_std::task;

/// Shared application state.
#[derive(Debug)]
struct State {
    name: String,
}

fn main() -> io::Result<()> {
    task::block_on(async {
        let mut app = tide::with_state(State { name: "nori".to_string() });
        app.at("/submit").post(|req: tide::Request<State>| {
            async move {
                let name = &req.state().name;
                tide::Response::new(200).body_string(name.to_string())
            }
        });
        app.listen("127.0.0.1:8080").await?;
        Ok(())
    })
}

I'd be happy to help you debug further if you could share a repro. I'm sorry this is happening, but thanks again for reporting!

@importcjj
Copy link
Author

@yoshuawuyts

My Rust version

stable-x86_64-unknown-linux-gnu (default)
rustc 1.39.0 (4560ea788 2019-11-04)

Here is a simple demo.

https://github.com/importcjj/tide-demo/blob/master/src/main.rs

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Dec 17, 2019

@importcjj I updated to a recent nightly version, and the diagnostics were super helpful! Solved the problem, and included the code. The short version is: req.pool needed to be assigned to a temporary, which fixed the trait resolver problems.

I'm not exactly sure what caused this to trip up in a recent Tide version, but the problem is easily sidestepped. It seems somewhat similar to the lifetime inference changes that happened as part of 1.39-nightly (rust-lang/rust#64292), but you mentioned you'd been using 1.39-stable all along so I really don't know.

Happy this seems to have been resolved though. Let me know how this works out for you, and then I think we should be okay to close this. Thanks!

Screenshot

2019-12-17-175740_1920x1080

Code

use async_std::io;
use async_std::task;

use mobc::{ConnectionManager, runtime::DefaultExecutor, Pool, AnyFuture};

struct FooManager;

struct FooConnection;

impl FooConnection {
    async fn query(&self) -> String {
        "nori".to_string()
    }
}

impl ConnectionManager for FooManager {
    type Connection = FooConnection;
    type Error = std::io::Error;
    type Executor = DefaultExecutor;

    fn get_executor(&self) -> Self::Executor {
        DefaultExecutor::current()
    }

    fn connect(&self) -> AnyFuture<Self::Connection, Self::Error> {
        Box::pin(futures::future::ok(FooConnection))
    }

    fn is_valid(&self, conn: Self::Connection) -> AnyFuture<Self::Connection, Self::Error> {
        Box::pin(futures::future::ok(conn))
    }

    fn has_broken(&self, conn: &mut Option<Self::Connection>) -> bool {
        false
    }
}

/// Shared application state.
struct State {
    pool: Pool<FooManager>
}

fn main() -> io::Result<()> {
    task::block_on(async {
        let pool = Pool::new(FooManager).await.unwrap();
        let mut app = tide::with_state(State { pool });
        app.at("/submit").post(|req: tide::Request<State>| {
            async move {
                let pool = &req.state().pool;
                let conn = pool.get().await.unwrap();
                let name = conn.query().await;
                tide::Response::new(200).body_string("hello".to_string())
            }
        });
        app.listen("127.0.0.1:8080").await?;
        Ok(())
    })
}

@estebank
Copy link

req.pool needed to be assigned to a temporary, which fixed the trait resolver problems.

@yoshuawuyts I know you said the ping wasn't a bug report, but I see that even though that change will be obvious to anyone reading the output with a moderate level of experience with Rust, that hint is not in the output and can be beyond the level of understanding of someone just starting out (or a decaffeinated experienced dev). That makes me think we might want to add a help: consider assigning this to a temporary variable message at the bottom in rustc for these cases. What do you think? If you agree, would you mind opening a ticket for it?

@yoshuawuyts
Copy link
Member

@estebank done!

@importcjj
Copy link
Author

Thank you!
But this error message really confused me.

@estebank
Copy link

@importcjj is the new error understandable to you? If not, please join the thread at the linked rust ticket.

@importcjj
Copy link
Author

@estebank The new error message is clear enough, but I'm just confused about the tide state() API.

@yoshuawuyts Why not just use req.state.pool instead of req.state().pool? After all, they do the same thing, and the former doesn't cause errors.

@yoshuawuyts
Copy link
Member

@importcjj providing public field access, unless directly modifiable as part of a constructor, is somewhat of an anti-pattern. Not too thrilled about type inference having a bad time here, but my hopes is that as work on async/await continues, inference will eventually catch up.

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

No branches or pull requests

3 participants