blackbeam / mysql_async

Asyncronous Rust Mysql driver based on Tokio.
Apache License 2.0
377 stars 114 forks source link

Regression in 0.32: Pool::disconnect does not wait for conns to be dropped anymore #243

Closed cloneable closed 1 year ago

cloneable commented 1 year ago

The following assert fails in a test program with 0.32. Works fine with 0.31.

https://github.com/blackbeam/mysql_async/blob/bbf24b0006bae6364714cfdad5f20ffe13b6af51/src/conn/pool/recycler.rs#L228-L229

#[tokio::main]
async fn main() -> mysql_async::Result<()> {
    let pool = mysql_async::Pool::new("mysql://root:password@127.0.0.1:3307/mysql");

    tokio::spawn({
        let pool = pool.clone();
        async move {
            let mut _conn = pool.get_conn().await.unwrap();
            tokio::time::sleep(std::time::Duration::from_secs(5)).await;
        }
    });

    pool.disconnect().await?;

    Ok(())
}

https://github.com/blackbeam/mysql_async/blob/bbf24b0006bae6364714cfdad5f20ffe13b6af51/src/conn/pool/mod.rs#L244-L248

blackbeam commented 1 year ago

@cloneable, hi.

I can't reproduce the issue so it's hard to know whether 8c4b72a fixes it. Could you please try to reproduce on the updated master?

cloneable commented 1 year ago

@blackbeam, sorry, the code snippet above was bad and has a race condition. Please see if you can reproduce it with this taken from the README. The fix to master did not help.

use std::error::Error as StdError;
use tokio::task::JoinHandle;

#[tokio::main]
async fn main() -> Result<(), Box<dyn StdError + 'static>> {
    let pool = mysql_async::Pool::new("mysql://root:password@127.0.0.1:3307/mysql");

    let task: JoinHandle<mysql_async::Result<()>> = tokio::spawn({
        let pool = pool.clone();
        let mut conn = pool.get_conn().await?;
        async move {
            use mysql_async::prelude::*;

            #[derive(Debug, PartialEq, Eq, Clone)]
            struct Payment {
                customer_id: i32,
                amount: i32,
                account_name: Option<String>,
            }

            let payments = vec![Payment {
                customer_id: 1,
                amount: 2,
                account_name: None,
            }];

            r"CREATE TEMPORARY TABLE payment (
                    customer_id int not null,
                    amount tinyint(3) not null,
                    account_name text
                )"
            .ignore(&mut conn)
            .await?;

            r"INSERT INTO payment (customer_id, amount, account_name)
                  VALUES (:customer_id, :amount, :account_name)"
                .with(payments.iter().map(|payment| {
                    params! {
                        "customer_id" => payment.customer_id,
                        "amount" => payment.amount,
                        "account_name" => payment.account_name.as_ref(),
                    }
                }))
                .batch(&mut conn)
                .await?;

            drop(conn);

            Ok(())
        }
    });

    task.await??;

    pool.disconnect().await?;

    Ok(())
}
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `0`', $HOME/.cargo/git/checkouts/mysql_async-a79c69708a1aa355/8c4b72a/src/conn/pool/recycler.rs:212:13
stack backtrace:
   0: rust_begin_unwind
             at /rustc/9eb3afe9ebe9c7d2b84b71002d44f4a0edac95e0/library/std/src/panicking.rs:575:5
   1: core::panicking::panic_fmt
             at /rustc/9eb3afe9ebe9c7d2b84b71002d44f4a0edac95e0/library/core/src/panicking.rs:64:14
   2: core::panicking::assert_failed_inner

If it matters, I'm currently testing against MariaDB 10.3.16. Gonna test against a newer one.

cloneable commented 1 year ago

Same with MariaDB 10.11.2. Found this in the logs: [Warning] Aborted connection 4 to db: 'mysql' user: 'root' host: 'x.x.x.x' (Got an error reading communication packets), but that's probably due to the rust binary terminating.

blackbeam commented 1 year ago

Thanks for the snippet. f23dca0 seem to fix it. Could you please also confirm?

cloneable commented 1 year ago

Yes, this seems to work for me, too.

cloneable commented 1 year ago

Thank you! Can be closed. Could you yank 0.32.0 from crates.io, so no one uses it?