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

Update CONTRIBUTING.md with proper LOOM test invocation #6515

Closed
wants to merge 1 commit into from

Conversation

adelavegaf
Copy link

Loom tests currently fail when LOOM_MAX_PREEMPTION is set to 1. Specifically, yield_calls_park_before_scheduling_again across all schedulers (current thread, multi thread, multi thread alt).

For example:

RUSTFLAGS="-Dwarnings --cfg loom  -C debug_assertions" LOOM_MAX_PREEMPTIONS=1 LOOM_MAX_BRANCHES=10000 cargo test --lib --release --features full -- --nocapture loom_current_thread
    Finished release [optimized] target(s) in 0.05s
     Running unittests src/lib.rs (/Users/ant/Developer/oss/tokio/target/release/deps/tokio-32b7cb1c593730d4)

running 3 tests
test runtime::tests::loom_current_thread::assert_no_unnecessary_polls ... ok
thread 'runtime::tests::loom_current_thread::yield_now::yield_calls_park_before_scheduling_again' panicked at tokio/src/runtime/tests/loom_current_thread/yield_now.rs:22:17:
assertion `left == right` failed
  left: 1
 right: 0
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'runtime::tests::loom_current_thread::yield_now::yield_calls_park_before_scheduling_again' panicked at /Users/ant/.cargo/registry/src/index.crates.io-6f17d22bba15001f/loom-0.7.1/src/rt/execution.rs:216:13:
deadlock; threads = [(Id(0), Blocked(Location(None))), (Id(1), Blocked(Location(None))), (Id(2), Blocked(Location(None)))]
thread 'runtime::tests::loom_current_thread::yield_now::yield_calls_park_before_scheduling_again' panicked at /Users/ant/.cargo/registry/src/index.crates.io-6f17d22bba15001f/loom-0.7.1/src/rt/thread.rs:276:39:
called `Option::unwrap()` on a `None` value
stack backtrace:
   0:        0x104dfbfe8 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::h3be43c6214e69043
   1:        0x104e19a70 - core::fmt::write::h12f9049830901a82
   2:        0x104df9bbc - std::io::Write::write_fmt::h2ff514dfe799c882
   3:        0x104dfbe1c - std::sys_common::backtrace::print::h722d6ff0889d39c2
   4:        0x104dfd958 - std::panicking::default_hook::{{closure}}::hd7f9985c1b9ae0aa
   5:        0x104dfd6a0 - std::panicking::default_hook::h29fb35819c2a32ca
   6:        0x104dfdecc - std::panicking::rust_panic_with_hook::h401df0236a0e7dae
   7:        0x104dfdc5c - std::panicking::begin_panic_handler::{{closure}}::h67f4ee7322151c3c
   8:        0x104dfc46c - std::sys_common::backtrace::__rust_end_short_backtrace::hbce128a2ff88406e
   9:        0x104dfda10 - _rust_begin_unwind
  10:        0x104e3192c - core::panicking::panic_fmt::hecc1dfe94cb15bc7
  11:        0x104e319b4 - core::panicking::panic::h73b079eecd9000fe
  12:        0x104e318b0 - core::option::unwrap_failed::h2a1d2a89129dbc81
  13:        0x104cd4880 - loom::rt::object::Ref<T>::set_action::h606daf31b2ba97a6
  14:        0x104cadf58 - scoped_tls::ScopedKey<T>::with::h634eac6aaf7668ce
  15:        0x104cb6d64 - loom::rt::atomic::Atomic<T>::store::he9584501adecba00
error: test failed, to rerun pass `--lib`

Caused by:
  process didn't exit successfully: `/Users/ant/Developer/oss/tokio/target/release/deps/tokio-32b7cb1c593730d4 --nocapture loom_current_thread` (signal: 10, SIGBUS: access to undefined memory)

Although the test consistently fails with this invocation (tried on my Mac and Linux using the repo's main branch), it's not failing on CI.

When looking into how we invoke loom in CI, I noticed we pass LOOM_MAX_PREEMPTIONS=2 (e.g. https://github.com/tokio-rs/tokio/actions/runs/8814945156/job/24195889256). This env var change makes the test pass.

Motivation

Loom tests fail when following the contribution guide on the main branch, despite them succeeding in CI. This is confusing for new comers like me!

Solution

This change updates the sample invocation provided in the contributing guide to match what we do in CI to ensure all loom tests pass.

Loom tests currently fail when `LOOM_MAX_PREEMPTION` is set to 1. Specifically, `yield_calls_park_before_scheduling_again` across all schedulers (current thread, multi thread, multi thread alt).

For example: 

```
RUSTFLAGS="-Dwarnings --cfg loom  -C debug_assertions" LOOM_MAX_PREEMPTIONS=1 LOOM_MAX_BRANCHES=10000 cargo test --lib --release --features full -- --nocapture loom_current_thread
    Finished release [optimized] target(s) in 0.05s
     Running unittests src/lib.rs (/Users/ant/Developer/oss/tokio/target/release/deps/tokio-32b7cb1c593730d4)

running 3 tests
test runtime::tests::loom_current_thread::assert_no_unnecessary_polls ... ok
thread 'runtime::tests::loom_current_thread::yield_now::yield_calls_park_before_scheduling_again' panicked at tokio/src/runtime/tests/loom_current_thread/yield_now.rs:22:17:
assertion `left == right` failed
  left: 1
 right: 0
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'runtime::tests::loom_current_thread::yield_now::yield_calls_park_before_scheduling_again' panicked at /Users/ant/.cargo/registry/src/index.crates.io-6f17d22bba15001f/loom-0.7.1/src/rt/execution.rs:216:13:
deadlock; threads = [(Id(0), Blocked(Location(None))), (Id(1), Blocked(Location(None))), (Id(2), Blocked(Location(None)))]
thread 'runtime::tests::loom_current_thread::yield_now::yield_calls_park_before_scheduling_again' panicked at /Users/ant/.cargo/registry/src/index.crates.io-6f17d22bba15001f/loom-0.7.1/src/rt/thread.rs:276:39:
called `Option::unwrap()` on a `None` value
stack backtrace:
   0:        0x104dfbfe8 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::h3be43c6214e69043
   1:        0x104e19a70 - core::fmt::write::h12f9049830901a82
   2:        0x104df9bbc - std::io::Write::write_fmt::h2ff514dfe799c882
   3:        0x104dfbe1c - std::sys_common::backtrace::print::h722d6ff0889d39c2
   4:        0x104dfd958 - std::panicking::default_hook::{{closure}}::hd7f9985c1b9ae0aa
   5:        0x104dfd6a0 - std::panicking::default_hook::h29fb35819c2a32ca
   6:        0x104dfdecc - std::panicking::rust_panic_with_hook::h401df0236a0e7dae
   7:        0x104dfdc5c - std::panicking::begin_panic_handler::{{closure}}::h67f4ee7322151c3c
   8:        0x104dfc46c - std::sys_common::backtrace::__rust_end_short_backtrace::hbce128a2ff88406e
   9:        0x104dfda10 - _rust_begin_unwind
  10:        0x104e3192c - core::panicking::panic_fmt::hecc1dfe94cb15bc7
  11:        0x104e319b4 - core::panicking::panic::h73b079eecd9000fe
  12:        0x104e318b0 - core::option::unwrap_failed::h2a1d2a89129dbc81
  13:        0x104cd4880 - loom::rt::object::Ref<T>::set_action::h606daf31b2ba97a6
  14:        0x104cadf58 - scoped_tls::ScopedKey<T>::with::h634eac6aaf7668ce
  15:        0x104cb6d64 - loom::rt::atomic::Atomic<T>::store::he9584501adecba00
error: test failed, to rerun pass `--lib`

Caused by:
  process didn't exit successfully: `/Users/ant/Developer/oss/tokio/target/release/deps/tokio-32b7cb1c593730d4 --nocapture loom_current_thread` (signal: 10, SIGBUS: access to undefined memory)
```

Although the test consistently fails with this invocation (tried on my Mac and Linux using the repo's main branch), it's not failing on CI.

When looking into how we invoke loom in CI, I noticed we pass LOOM_MAX_PREEMPTIONS=2 (e.g. https://github.com/tokio-rs/tokio/actions/runs/8814945156/job/24195889256). This env var change makes the test pass.

And so this diff updates the invocation to match what we do in CI.
@Darksonn
Copy link
Contributor

This is a bug in the test, and ideally we fix the test.

@Darksonn
Copy link
Contributor

Darksonn commented May 4, 2024

Closing in favor of #6537.

We don't want to change the command, because increasing LOOM_MAX_PREEMPTIONS makes the tests take a lot longer.

@Darksonn Darksonn closed this May 4, 2024
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