blackbeam / rust-mysql-simple

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

Return None when QueryResult::state is Errored on next_set() #285

Closed gliderkite closed 3 years ago

gliderkite commented 3 years ago

Where I work we use this library quite a lot (thanks for the work!), but we stumbled against an issue that I can reproduce also on the latest version of this library (currently 21.0.0).

Basically when executing an invalid query the code hangs and it's fortunately reproducible 100% of the time. The code that triggers the issue looks like the following:

// 1. Create database
// 2. Create user with privileges

let opts = OptsBuilder::new()
    .ip_or_hostname(my_ip)
    .user(Some(my_user))
    .pass(Some(my_password))
    .db_name(Some(my_db));

let mut conn = Conn::new(opts).unwrap();

// NOTE: the last of the VALUES is missing!
let invalid_sql = r#"
CREATE TABLE IF NOT EXISTS `user_details` (
    `user_id` int(11) NOT NULL AUTO_INCREMENT,
    `username` varchar(255) DEFAULT NULL,
    `first_name` varchar(50) DEFAULT NULL,
    `last_name` varchar(50) DEFAULT NULL,
    PRIMARY KEY (`user_id`)
);

INSERT INTO `user_details` (`user_id`, `username`, `first_name`, `last_name`)
VALUES (1, 'rogers63', 'david')
"#;

let result_set = conn.query_iter(invalid_sql).unwrap();

After a bit of debugging I noticed that it hangs on QueyResult::drop, in particular at the 3rd call of QueryResult::next_set.

This MR is a first attempt to solve the issue, we are currently using this patch since it fixes the issue, however I do not have enough context to understand the consequences of this change, nor currently the time to implement some tests. Please feel free to merge if you think this is indeed a fix, to take it over, or to guide me over the following steps.

blackbeam commented 3 years ago

Hi, thanks for PR! I'm on a trip at the moment, so I'll look into it a bit later.

gliderkite commented 3 years ago

Hi, thanks. Please note that I also tried to return Some(Err(error)) instead (I had to create a dummy error since the current one cannot be cloned). But it also hangs in that case.

blackbeam commented 3 years ago

This is superseded by e5db79d. Closing. Thanks!