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 spurious yield_defers_until_park test #5634

Merged
merged 2 commits into from
Apr 20, 2023
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 48 additions & 7 deletions tokio/tests/rt_common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -695,12 +695,34 @@ rt_test! {
/// Tests that yielded tasks are not scheduled until **after** resource
/// drivers are polled.
///
/// Note: we may have to delete this test as it is not necessarily reliable.
/// The OS does not guarantee when I/O events are delivered, so there may be
/// more yields than anticipated.
/// more yields than anticipated. This makes the test slightly flaky. To
/// help avoid flakiness, we run the test 10 times and only fail it after
/// 10 failures in a row.
///
/// Note that if the test fails by panicking rather than by returning false,
/// then we fail it immediately. That kind of failure should not happen
/// spuriously.
#[test]
#[cfg(not(target_os="wasi"))]
fn yield_defers_until_park() {
for _ in 0..10 {
if yield_defers_until_park_inner() {
// test passed
return;
}

// Wait a bit and run the test again.
std::thread::sleep(std::time::Duration::from_secs(2));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any rationale for 2 seconds in particular? wondering if we could get away with a shorter sleep so this test doesn't take 20s to run...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It only sleeps if it actually fails, which is very rare.

Presumably if this fails, that's because the kernel is too busy handling other tests to deliver the IO event. I picked 2 seconds since the other tests in this file are likely to be done by then.

I don't think the duration it takes when we've broken the test is important.

}

panic!("yield_defers_until_park is failing consistently");
}

/// Implementation of `yield_defers_until_park` test. Returns `true` if the
/// test passed.
#[cfg(not(target_os="wasi"))]
fn yield_defers_until_park_inner() -> bool {
use std::sync::atomic::{AtomicBool, Ordering::SeqCst};
use std::sync::Barrier;

Expand All @@ -727,14 +749,16 @@ rt_test! {

barrier.wait();

tokio::spawn(async move {
let (fail_test, fail_test_recv) = oneshot::channel::<()>();

let jh = tokio::spawn(async move {
// Create a TCP litener
let listener = TcpListener::bind("127.0.0.1:0").await.unwrap();
let addr = listener.local_addr().unwrap();

tokio::join!(
async {
// Done blocking intentionally
// Done in a blocking manner intentionally.
let _socket = std::net::TcpStream::connect(addr).unwrap();

// Yield until connected
Expand All @@ -744,7 +768,12 @@ rt_test! {
cnt += 1;

if cnt >= 10 {
panic!("yielded too many times; TODO: delete this test?");
// yielded too many times; report failure and
// sleep forever so that the `fail_test` branch
// of the `select!` below triggers.
let _ = fail_test.send(());
futures::future::pending::<()>().await;
break;
}
}
},
Expand All @@ -753,8 +782,20 @@ rt_test! {
flag.store(true, SeqCst);
}
);
}).await.unwrap();
});
});

// Wait until the spawned task completes or fails. If no message is
// sent on `fail_test`, then the test succeeds. Otherwise, it fails.
let success = fail_test_recv.await.is_err();

if success {
// Check for panics in spawned task.
jh.abort();
jh.await.unwrap();
}

success
})
}

#[cfg(not(target_os="wasi"))] // Wasi does not support threads
Expand Down