blackbeam / rust-mysql-simple

Mysql client library implemented in rust.
Apache License 2.0
663 stars 144 forks source link

`reset()` panics if called on borked `mysql::Conn` #317

Closed fulara closed 2 years ago

fulara commented 2 years ago

Don't have an exact reproduction steps but this should be something like this:

1) Start target db 2) Create mysql::Conn() and make sure it's able to do some queries. 3) shutdown target db 4) Code that executed query on the mysql conn now Errors out. 5) In the Error handler invoke conn.reset() This causes a panic with "incomplete connection" message in it.

That was first time I used the reset() function, I'll just stick to recreating the conn altogether - maybe that's expected usage, but found above a bit surprising.

blackbeam commented 2 years ago

Hi.

This seem strange. I couldn't reproduce the error. Is it possible for you to get a backtrace?

fulara commented 2 years ago

Hello!

Apologies, issue reproduces in my environment 100% of the cases, although I am only using it against my real prod like scenario, i did not bother to produce mcve - sorry!

Few additional notes, not sure if they matter much but: 1) I am executing multi statement queries 2) this is fn that actually executes the queries:

pub fn exec_multi_statement_query(conn: &mut mysql::Conn, query: &str) -> anyhow::Result<()> {
    let result: anyhow::Result<()> = try {
        let mut result = conn.query_iter(query)?;
        while let Some(result_set) = result.iter() {
            for row in result_set {
                row?;
            }
        }
    };
    if result.is_err() {
        if let Err(e) = conn.query_drop("ROLLBACK") {
            // logging e here...
            conn.reset()?; // <<< this panics. but luckily is not necessary in my code.
        }
        result?;
    }
    Ok(())
}

below is the stack:

                               at /rustc/777bb86bcdbc568be7cff6eeeaaf81a89b4aa50b/library/core/src/option.rs:715:21
  21:     0x5635a79c5ff9 - mysql::conn::Conn::stream_mut::he637066580ae75eb
                               at /xyz/mysql-22.1.0/src/conn/mod.rs:283:9
  22:     0x5635a79cc71e - mysql::conn::Conn::reset_seq_id::hcac53cf284f25566
                               at /xyz/mysql-22.1.0/src/conn/mod.rs:785:9
  23:     0x5635a79cc980 - mysql::conn::Conn::write_command::hb22d69e0576dfd64
                               at /xyz/mysql-22.1.0/src/conn/mod.rs:806:9
  24:     0x5635a79ce631 - mysql::conn::Conn::_query::h9bfd4bde978c4c34
                               at /xyz/mysql-22.1.0/src/conn/mod.rs:961:9
  25:     0x5635a79cf498 - <mysql::conn::Conn as mysql::conn::queryable::Queryable>::query_iter::h15bf45012e4d9e59
                               at /xyz/mysql-22.1.0/src/conn/mod.rs:1145:20
  26:     0x5635a7643963 - <my code calls .reset()>

Is that sufficient?

blackbeam commented 2 years ago

Is that sufficient?

Not sure. Is it somehow possible that reset is called multiple times?

fulara commented 2 years ago

sort of - there are multiple threads in parallel that are pumping data to database.

but on the same conn, I don't think so.

fulara commented 2 years ago

Actually looking at the code yes. This may happen.

I only update the conn if the establishing of new conn has succeeded, if it did not it will use try to use the same conn, so my bad.

blackbeam commented 2 years ago

Anyway, code assumes that the stream is always available, it's a bug. Thanks for report!