witnet / witnet-rust

Open source Rust implementation of the Witnet decentralized oracle protocol, including full node and wallet backend 👁️🦀
https://docs.witnet.io
GNU General Public License v3.0
179 stars 56 forks source link

Fix race condition in config tests #144

Closed tmpolaczyk closed 5 years ago

tmpolaczyk commented 5 years ago

Using the following command to run tests until they fail:

while true; do cargo test --all -- --test-threads=30; VAR=$?; if [ $VAR -ne 0 ]; then break; fi; done

We can see a failure in loaders::toml::tests::test_load_config_from_file:

     Running target/debug/deps/witnet_config-fc324e1544e1c760

running 12 tests
test config::tests::test_storage_default_from_partial ... ok
test config::tests::test_config_default_from_partial ... ok
test config::tests::test_connections_default_from_partial ... ok
test config::tests::test_storage_from_partial ... ok
test config::tests::test_connections_from_partial ... ok
test loaders::toml::tests::test_load_empty_config_from_file ... ok
test loaders::toml::tests::test_load_empty_config ... ok
test loaders::toml::tests::test_load_durations ... ok
test loaders::toml::tests::test_load_config_from_file ... FAILED
test loaders::toml::tests::test_configure_environment ... ok
test loaders::toml::tests::test_configure_storage ... ok
test loaders::toml::tests::test_configure_connections ... ok

failures:

---- loaders::toml::tests::test_load_config_from_file stdout ----
thread 'loaders::toml::tests::test_load_config_from_file' panicked at 'assertion failed: `(left == right)`
  left: `None`,
 right: `Some(999)`', config/src/loaders/toml.rs:103:9
note: Run with `RUST_BACKTRACE=1` for a backtrace.

failures:
    loaders::toml::tests::test_load_config_from_file

test result: FAILED. 11 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

error: test failed, to rerun pass '--lib'

This is a race condition so it may take a while to replicate the bug.

The reason is the use of unsafe code in config/loaders/toml.rs:

static mut FILE_CONTENTS: &str = "";

    #[test]
    fn test_load_config_from_file() {

        unsafe {
            super::FILE_CONTENTS = r"
environment = 'testnet-1'
[connections]
inbound_limit = 999
"
        }

A simple fix would be to load configuration from a string instead.

To safely mutate global variables in Rust, use RwLock.

aesedepece commented 5 years ago

@tmpolaczyk putting a lock on FILE_CONTENTS just for it to be testable looks a little overkill but it won't do any harm any way, so it sounds like a reasonable compromise to me.

anler commented 5 years ago

This is a race condition, not a data race, so any kind of synchronization around the global mutable state won't solve the problem, well, a Mutex will, but that's not necessary at all, I've created #195 which should solve it by making the global mutable state thread local.

aesedepece commented 5 years ago

Solved by #195