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

calling Pool::close doesn't close connection #897

Closed
PrivateRookie opened this issue Dec 13, 2020 · 5 comments
Closed

calling Pool::close doesn't close connection #897

PrivateRookie opened this issue Dec 13, 2020 · 5 comments

Comments

@PrivateRookie
Copy link

PrivateRookie commented Dec 13, 2020

after calling pool.close and process exits, mysql log message

2020-12-13T14:05:14.091856Z 11 [Note] Aborted connection 11 to db: 'sqlx_test' user: 'root' host: '172.17.0.1' (Got an error reading communication packets)

which indicates that sqlx doesn't send QUIT command to mysql and close tcp connection directly.

test env

mysql 5.7, start command docker run -d --name mysql -p 3307:3306 -e MYSQL_ROOT_PASSWORD=1234TttT mysql:5.7

rust code

deps

sqlx = { version = "0.4.1", features = ["runtime-tokio-rustls", "mysql", "all-types", "bigdecimal"] }
tokio = { version = "0.2.23", features = ["full"] }
use sqlx::mysql::MySqlSslMode;
use sqlx::{
    mysql::{MySqlConnectOptions, MySqlPoolOptions},
    MySqlPool,
};
#[tokio::main]
async fn main() -> Result<(), sqlx::Error> {
    let opts = MySqlConnectOptions::new()
        .ssl_mode(MySqlSslMode::Disabled)
        .host("127.0.0.1")
        .port(3307)
        .username("root")
        .password("1234TttT")
        .database("sqlx_test");
    let pool = MySqlPool::connect_with(opts).await?;
    let output: Vec<(String,)> = sqlx::query_as("SHOW TABLES").fetch_all(&pool).await?;
    println!("{:?}", output);
    pool.close().await;
    Ok(())
}

after digging into sqlx code, I found that impl of SharedPool pub(super) async fn close(&self)

seem to forget calling close. after fixing like

    pub(super) async fn close(&self) {
        self.is_closed.store(true, Ordering::Release);
        while let Ok(waker) = self.waiters.pop() {
            waker.wake();
        }

        // ensure we wait until the pool is actually closed
        while self.size() > 0 {
            let _ = self
                .idle_conns
                .pop()
                .map(|idle| Floating::from_idle(idle, self))
                .map(|f| async { f.close().await });

            // yield to avoid starving the executor
            sqlx_rt::yield_now().await;
        }
    }

warning from mysql is gone

@abonander
Copy link
Collaborator

            let _ = self
                .idle_conns
                .pop()
                .map(|idle| Floating::from_idle(idle, self))
                .map(|f| async { f.close().await });

This doesn't actually call .close() on the connection. Remember that futures in Rust are lazily evaluated. Here, the async block that calls f.close().await; is never itself .awaited which means it does nothing besides drop the connection anyway.

To ensure .close() is called, you need to .await it in the context of the function itself, e.g.:

if let Some(idle) = self.idle_conns.pop() {
    let _ = Floating::from_idle(idle, self).close().await;
}

which looks better anyway.

@PrivateRookie
Copy link
Author

In fact, .close() has been call, because |f| async ( f.close().await }) is async block rather that async closure, according to
rfc - async_await#async block, async block is invoked immediately.

I wrote in this style because it seems to working and it works after tested. But i agree that it's a little confusing.
Anyway, @andrewwhitehead pr use style you prefer

@abonander
Copy link
Collaborator

That is an old RFC and is not exactly representative of the current implementation of async/await; even so, it does not purport to demonstrate that async blocks are eagerly evaluated. It's only intended as an example of how to declare one. You need to pin and .poll() an async block for it to execute, which is done either by spawning it into a runtime or .awaiting it in an async block or function that's executing within a runtime.

If you still don't believe me, see for yourself: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=a3f6b6336f79dcc5df4adf721e9f9102

@PrivateRookie
Copy link
Author

Thank you, I got your point, you can close this issue as you like

@abonander
Copy link
Collaborator

Closed by #895

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

2 participants