blackbeam / rust-mysql-simple

Mysql client library implemented in rust.
Apache License 2.0
665 stars 146 forks source link

Fix get_system_var() may panic when query() returns Err #190

Closed evenyag closed 5 years ago

evenyag commented 5 years ago

Conn::get_system_var() unwraps the Result from Conn::query(), which may be an Err and cause panic. This PR just returns the Err to caller and avoids panic.

The backtrace:

panicked 'called `Result::unwrap()` on an `Err` value: IoError { Resource temporarily unavailable (os error 11) }' at "src/libcore/result.rs:999"
stack backtrace:
   0:     0x7fe24c3e37ed - backtrace::backtrace::libunwind::trace::hc337d5444fd0a0f5
                        at /root/.cargo/registry/src/gitlab.alipay-inc.com-bfbf2811318eff5d/backtrace-0.2.3/src/backtrace/libunwind.rs:54
                         - backtrace::backtrace::trace::ha937d415e37244c5
                        at /root/.cargo/registry/src/gitlab.alipay-inc.com-bfbf2811318eff5d/backtrace-0.2.3/src/backtrace/mod.rs:70
   1:     0x7fe24c3e35f2 - backtrace::capture::Backtrace::new::h97acc86c3eaf11ae
                        at /app/cse/target/release/build/backtrace-dbd0bf3110fc6ba9/out/capture.rs:79
                         - <backtrace::capture::Backtrace as core::default::Default>::default::h12639b0bed54453a
                        at /app/cse/target/release/build/backtrace-dbd0bf3110fc6ba9/out/capture.rs:187
   2:     0x7fe24b9bef5e - cse::engine::util::set_exit_hook::{{closure}}::h9099a23bf5c8be3c
                        at src/engine/util/mod.rs:153
   3:     0x7fe24c4fc108 - std::panicking::rust_panic_with_hook::hffcefc09751839d1
                        at src/libstd/panicking.rs:481
   4:     0x7fe24c4fbba1 - std::panicking::continue_panic_fmt::hc0f142c930c846fc
                        at src/libstd/panicking.rs:384
   5:     0x7fe24c4fba85 - rust_begin_unwind
                        at src/libstd/panicking.rs:311
   6:     0x7fe24c51a09c - core::panicking::panic_fmt::h2daf88b2616ca2b2
                        at src/libcore/panicking.rs:85
   7:     0x7fe24bf065cd - core::result::unwrap_failed::h3d5513ad73ce9fec
                        at /rustc/eae3437dfe991621e8afdc82734f4a172d7ddf9b/src/libcore/macros.rs:18
   8:     0x7fe24befe5ae - core::result::Result<T,E>::unwrap::h3057531dd3b1b7fa
                        at /rustc/eae3437dfe991621e8afdc82734f4a172d7ddf9b/src/libcore/result.rs:800
                         - mysql::conn::Conn::get_system_var::h22290d04748e1b3d
                        at /root/.cargo/git/checkouts/rust-mysql-simple-dc00ed2e26128ed0/c10c7e9/src/conn/mod.rs:1760
   9:     0x7fe24befd756 - mysql::conn::Conn::connect::{{closure}}::hba7c599bf9084885
                        at /root/.cargo/git/checkouts/rust-mysql-simple-dc00ed2e26128ed0/c10c7e9/src/conn/mod.rs:1744
                         - core::result::Result<T,E>::and_then::h1b9d43fd86cc258b
                        at /rustc/eae3437dfe991621e8afdc82734f4a172d7ddf9b/src/libcore/result.rs:639
                         - mysql::conn::Conn::connect::h9409ef92d421571b
                        at /root/.cargo/git/checkouts/rust-mysql-simple-dc00ed2e26128ed0/c10c7e9/src/conn/mod.rs:1741
  10:     0x7fe24bef6f98 - mysql::conn::Conn::hard_reset::h81bd2816d1e85c13
                        at /root/.cargo/git/checkouts/rust-mysql-simple-dc00ed2e26128ed0/c10c7e9/src/conn/mod.rs:787
  11:     0x7fe24bef70f7 - mysql::conn::Conn::reset::{{closure}}::hf7313c9013506c37
                        at /root/.cargo/git/checkouts/rust-mysql-simple-dc00ed2e26128ed0/c10c7e9/src/conn/mod.rs:794
                         - core::result::Result<T,E>::or_else::h7d8a5249ae34f3a7
                        at /rustc/eae3437dfe991621e8afdc82734f4a172d7ddf9b/src/libcore/result.rs:709
                         - mysql::conn::Conn::reset::h53312e6ee5923882
                        at /root/.cargo/git/checkouts/rust-mysql-simple-dc00ed2e26128ed0/c10c7e9/src/conn/mod.rs:794
  12:     0x7fe24bf04b45 - mysql::conn::pool::Pool::_get_conn::hcb5d860b85b56eef
                        at /root/.cargo/git/checkouts/rust-mysql-simple-dc00ed2e26128ed0/c10c7e9/src/conn/pool.rs:183
  13:     0x7fe24bf05268 - mysql::conn::pool::Pool::try_get_conn::h62cb6ac2b64774a7
                        at /root/.cargo/git/checkouts/rust-mysql-simple-dc00ed2e26128ed0/c10c7e9/src/conn/pool.rs:242
blackbeam commented 5 years ago

Hi. Could you please rebase?

evenyag commented 5 years ago

Hi. Could you please rebase?

done

blackbeam commented 5 years ago

Note that the error is quite odd and this PR, actually, doesn't solve it. Looks like I should think of how to handle EAGAIN.

Could you please describe your environment? Is it VM?

blackbeam commented 5 years ago

I believe that this unwrap should be infallible. Also looks like there exist situations where it is possible for blocking socket to return EAGAIN. I'll investigate further.

evenyag commented 5 years ago

I believe that this unwrap should be infallible. Also looks like there exist situations where it is possible for blocking socket to return EAGAIN. I'll investigate further.

recv()/send() may returns EAGAIN when a read/write timeout had been set and the timeout expired. As described in man recv

EAGAIN or EWOULDBLOCK The socket is marked nonblocking and the receive operation would block, or a receive timeout had been set and the timeout expired before data was received. POSIX.1 allows either error to be returned for this case, and does not require these constants to have the same value, so a portable application should check for both possibilities.

When the db server is under heavy load, it may not be able to handle all client's request. It's reasonable to throws such error to the caller and let the caller decides what to do next (such as retry or reply Server is busy to user) instead of panic in get_system_var(), because even get_system_var() supports retry, errors may still occur.

blackbeam commented 5 years ago

Yeah. Panic is definitely not an option. However get_system_var() is requered to properly configure the connection. Could you please alter this PR to retry get_system_var() until success in case of EAGAIN (and keep error for other cases)?

evenyag commented 5 years ago

Since the read_timeout/write_timeout set by user had been expired in such situation (we're using blocking socket), it should be ok to just throw the error in get_system_var() and stop the Conn::new() process, keep the codes simple and clear. Also, the EAGAIN case rarely occurs in most user's environment