tursodatabase / libsql-client-rs

libSQL Rust client library can be used to communicate with sqld natively over HTTP protocol with native Rust interface.
MIT License
75 stars 29 forks source link

Reqwest client not playing nicely with `Send` bound #21

Closed Kazy closed 1 year ago

Kazy commented 1 year ago

Follow up from my question on Discord.

I'm trying to use the reqwest client with Shuttle, which requires the returned future to be Send. But even with an Arc and a Mutex, the future returned by execute doesn't seem to be Send.

Here is a reproduction:

use std::sync::Arc;

use axum::{routing::get, Router};
use libsql_client::{reqwest::Client, DatabaseClient};
use tokio::sync::{Mutex};

async fn hello_world() -> &'static str {
    "Hello, world!"
}

#[shuttle_runtime::main]
async fn axum() -> shuttle_axum::ShuttleAxum {
    let client = Arc::new(Mutex::new(Client::new("url", "token")));

    {
        let resp = client.clone()
            .lock()
            .await
            .execute(
                "create table if not exists example_users ( uid text primary key, email text );",
            )
            .await
            .unwrap();
    }

    let router = Router::new().route("/", get(hello_world));

    Ok(router.into())
}

with the following error:

error: future cannot be sent between threads safely
  --> src/main.rs:11:1
   |
11 | #[shuttle_runtime::main]
   | ^^^^^^^^^^^^^^^^^^^^^^^^ future returned by `loader` is not `Send`
   |
   = help: the trait `Send` is not implemented for `dyn Future<Output = Result<ResultSet, anyhow::Error>>`
note: future is not `Send` as it awaits another future which is not `Send`
  --> src/main.rs:16:20
   |
16 |           let resp = client
   |  ____________________^
17 | |             .lock()
18 | |             .await
19 | |             .execute(
20 | |                 "create table if not exists example_users ( uid text primary key, email text );",
21 | |             )
   | |_____________^ await occurs here on type `Pin<Box<dyn Future<Output = Result<ResultSet, anyhow::Error>>>>`, which is not `Send`
note: required by a bound in `start`
  --> /mnt/snd-linux/extend/projects/shuttle-batch/shuttle/runtime/src/alpha/mod.rs:52:33
   |
52 | pub async fn start(loader: impl Loader<ProvisionerFactory> + Send + 'static) {
   |                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `start`
   = note: this error originates in the attribute macro `shuttle_runtime::main` (in Nightly builds, run with -Z macro-backtrace for more info)

warning: `hello-world` (bin "hello-world") generated 2 warnings
error: could not compile `hello-world` (bin "hello-world") due to previous error; 2 warnings emitted

And Cargo.toml for completness:

[package]
name = "hello-world"
version = "0.1.0"
edition = "2021"

[dependencies]
axum = "0.6.10"
shuttle-axum = { version = "0.18.0" }
shuttle-runtime = { version = "0.18.0" }
libsql-client = "0.26.2"
tokio = { version = "1.26.0" }
psarna commented 1 year ago

Ah, right, ultimately it's all because the async_trait on top of which the clients are implemented is also marked ?Send. I'll try to rework the implementations to get rid of this constraint, and make the driver usable with shuttle

psarna commented 1 year ago

@Kazy I decided to just refactor the whole driver to drop async_trait, it didn't bring much value for our use case while making the API extremely complicated (: I just released 0.27.0 (https://crates.io/crates/libsql-client), and everything is also pushed to the main branch. Please let me know if the new version works for you!

I also entirely dropped the async DatabaseClient trait -- now there's only a libsql_client::Client enum, and it's not an async trait anymore. In order to explicitly create a reqwest client in your example, you could use an idiom like:

let client = Client::Reqwest(libsql_client::reqwest::Client::new("url", "token"));

... or, just use libsql_client::new_client()/libsql_client::new_client_from_config() and let the backend be picked automatically.

psarna commented 1 year ago

If something still needs to be polished to make the API usable, please let me know and I'll roll the fixes and release a new version!

Kazy commented 1 year ago

Thank you for the fast resolution, this works perfectly ! Btw the latest version of the doc is failing (https://docs.rs/crate/libsql-client/latest).

psarna commented 1 year ago

ah, will fix, I bet I forgot to update code samples in some of the doctests. Thanks!

birtles commented 1 year ago

Sorry to jump on an old issue here, I'm just wondering if something regressed or if I'm just holding it wrong. I thought the changes in https://github.com/libsql/libsql-client-rs/commit/9edd0637739768e4c17225bb5615442deb91644a#diff-7f93c4e263c4e9ec748f804c7fd04a3b2fde86ffd741fb5516d67e1097bae4c1R26 added:

unsafe impl Send for Client {}

which I thought meant Client should now be Send and the Arc / Mutex dance wasn't required but with the following code:

use lambda_http::{run, service_fn, Body, Error, Request, RequestExt, Response};
use libsql_client::{args, Statement};

async fn function_handler(event: Request) -> Result<Response<Body>, Error> {
    let db = libsql_client::Client::from_env().await?;

    db.execute(
        Statement::with_args(
            "SELECT ...",
            args!(format!("{q}*")),
        )
    ).await?;

    let resp = Response::builder()
        .status(200)
        .header("content-type", "text/plain")
        .body("OK".into())
        .map_err(Box::new)?;
    Ok(resp)
}

#[tokio::main]
async fn main() -> Result<(), Error> {
    run(service_fn(function_handler)).await
}

I get:

error: future cannot be sent between threads safely
   --> src/main.rs:112:9
    |
112 |     run(service_fn(function_handler)).await
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ future returned by `function_handler` is not `Send`
    |
    = help: within `libsql_client::Client`, the trait `Sync` is not implemented for `*mut libsql_sys::ffi
note: future is not `Send` as this value is used across an await
   --> src/main.rs:69:7
    |
63  |     db.execute(
    |     -- has type `&libsql_client::Client` which is not `Send`
...
69  |     ).await?;
    |       ^^^^^ - `db` is later dropped here
    |       |
    |       await occurs here, with `db` maybe used later
help: consider moving this into a `let` binding to create a shorter lived borrow
   --> src/main.rs:63:5
    |
63  | /     db.execute(
64  | |         Statement::with_args(
65  | |             "SELECT ...
..
67  | |             args!(format!("{q}*")),
68  | |         )
69  | |     ).await?;
    | |_____^
note: required by a bound in `lambda_http::run`
   --> /home/birtles/.cargo/registry/src/index.crates.io-6f17d22bba15001f/lambda_http-0.8.1/src/lib.rs:19
    |
188 | pub async fn run<'a, R, S, E>(handler: S) -> Result<(), Error>
    |              --- required by a bound in this function
...
191 |     S::Future: Send + 'a,
    |                ^^^^ required by this bound in `run`
birtles commented 1 year ago

My bad. I needed to disable the default local backend.

psarna commented 1 year ago

Yes, local backend is the main source of trouble with async multi-threaded environments, because it inherently uses raw pointers to access the C API.

Also, note that we're very actively working on a new generation client that was designed to work with Send environments from the start: https://github.com/tursodatabase/libsql/tree/main/crates/core . It still has some loose ends, but soon it will match libsql-client-rs in functionality, and then it will be the recommended choice for all use cases.

birtles commented 1 year ago

Also, note that we're very actively working on a new generation client that was designed to work with Send environments from the start: https://github.com/tursodatabase/libsql/tree/main/crates/core . It still has some loose ends, but soon it will match libsql-client-rs in functionality, and then it will be the recommended choice for all use cases.

Great, thank you. Is it still too early to switch to using that?

psarna commented 1 year ago

Depends on what parts of the API you rely on. We still haven't implemented things like batches and transaction structs fully yet, for instance: https://github.com/tursodatabase/libsql/blob/bea88634dbdf1f578c5187196e7e311f5aa8a67c/crates/core/src/hrana/mod.rs#L281 https://github.com/tursodatabase/libsql/blob/bea88634dbdf1f578c5187196e7e311f5aa8a67c/crates/core/src/hrana/mod.rs#L298

birtles commented 1 year ago

Depends on what parts of the API you rely on. We still haven't implemented things like batches and transaction structs fully yet, for instance:

Great, thank you very much. I'll keep watching the repo for updates to the crate.