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

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

Merged
merged 1 commit into from Nov 2, 2019

Conversation

evenyag
Copy link

@evenyag evenyag commented Oct 24, 2019

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
Copy link
Owner

Hi. Could you please rebase?

@evenyag
Copy link
Author

evenyag commented Oct 29, 2019

Hi. Could you please rebase?

done

@blackbeam
Copy link
Owner

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
Copy link
Owner

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
Copy link
Author

evenyag commented Oct 29, 2019

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
Copy link
Owner

blackbeam commented Oct 29, 2019

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
Copy link
Author

evenyag commented Oct 30, 2019

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

@blackbeam blackbeam merged commit 69620aa into blackbeam:master Nov 2, 2019
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

Successfully merging this pull request may close these issues.

None yet

2 participants