Windows: Use futex implementation for Once

[timokroeger]

Keep the queue implementation for win7.
Inspired by PR #121956

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Amanieu (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[timokroeger]

@rustbot label O-windows

[timokroeger]

r? @ChrisDenton

[workingjubilee]

cc @joboet who has touched related API including #99505

[ChrisDenton]

Thanks! Looks good to me, just one small nit.

[ChrisDenton]

Thanks again!

@bors r+

[bors]

📌 Commit fa58d1b has been approved by ChrisDenton

It is now in the queue for this repository.

[ChrisDenton]

@bors rollup=iffy

While I don't anticipate issues, changing the Once implementation could potentially cause problems so it shouldn't be rolled up with other PRs absolutely unless necessary.

[workingjubilee]

has been waiting for 3 days, it seems.
@bors p=3

[bors]

⌛ Testing commit fa58d1b with merge d79ed88...

[rust-log-analyzer]

The job x86_64-gnu-aux failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[bors]

💔 Test failed - checks-actions

[ChrisDenton]

The failure was in miri's tests, which look like they've found a genuine deadlock. Only it occurred in the thread parker (via mpmc), not in Once code:

error: deadlock: the evaluated program deadlocked
##[error]   --> /checkout/library/std/src/sys/sync/thread_parking/windows.rs:120:90
    |
120 |             c::WaitOnAddress(self.ptr(), &PARKED as *const _ as c::LPVOID, 1, c::INFINITE);
    |                                                                                          ^ the evaluated program deadlocked
    |
    = note: BACKTRACE:
    = note: inside `realstd::sys::sync::thread_parking::windows::Parker::park` at /checkout/library/std/src/sys/sync/thread_parking/windows.rs:120:90: 120:91
    = note: inside `realstd::thread::Thread::park` at /checkout/library/std/src/thread/mod.rs:1361:18: 1361:53
    = note: inside `realstd::thread::park` at /checkout/library/std/src/thread/mod.rs:1104:9: 1104:25
    = note: inside `realstd::sync::mpmc::context::Context::wait_until` at /checkout/library/std/src/sync/mpmc/context.rs:139:17: 139:31
    = note: inside closure at /checkout/library/std/src/sync/mpmc/list.rs:444:27: 444:50
    = note: inside closure at /checkout/library/std/src/sync/mpmc/context.rs:50:13: 50:18
    = note: inside closure at /checkout/library/std/src/sync/mpmc/context.rs:58:31: 58:37
    = note: inside `realstd::thread::LocalKey::<core::cell::Cell<core::option::Option<realstd::sync::mpmc::context::Context>>>::try_with::<{closure@realstd::sync::mpmc::context::Context::with<{closure@realstd::sync::mpmc::list::Channel<test::event::CompletedTest>::recv::{closure#1}}, ()>::{closure#1}}, ()>` at /checkout/library/std/src/thread/local.rs:283:12: 283:27
    = note: inside `realstd::sync::mpmc::context::Context::with::<{closure@realstd::sync::mpmc::list::Channel<test::event::CompletedTest>::recv::{closure#1}}, ()>` at /checkout/library/std/src/sync/mpmc/context.rs:53:9: 62:15
    = note: inside `realstd::sync::mpmc::list::Channel::<test::event::CompletedTest>::recv` at /checkout/library/std/src/sync/mpmc/list.rs:434:13: 455:15
    = note: inside `realstd::sync::mpmc::Receiver::<test::event::CompletedTest>::recv` at /checkout/library/std/src/sync/mpmc/mod.rs:307:43: 307:58
    = note: inside `realstd::sync::mpsc::Receiver::<test::event::CompletedTest>::recv` at /checkout/library/std/src/sync/mpsc/mod.rs:849:9: 849:26
    = note: inside `test::run_tests::<{closure@test::run_tests_console::{closure#2}}>` at /checkout/library/test/src/lib.rs:434:27: 434:36
    = note: inside `test::run_tests_console` at /checkout/library/test/src/console.rs:322:5: 322:70
    = note: inside `test::test_main` at /checkout/library/test/src/lib.rs:142:19: 142:59
    = note: inside `test::test_main_static` at /checkout/library/test/src/lib.rs:164:5: 164:40
    = note: inside `main`

This will need some investigation.

[timokroeger]

Reproducible on x86_64 windows 11 with: ./x miri --stage 1 library/std -- sync::once::tests::wait_for_force_to_finish
When skipping the wait_for_force_to_finish test case everything else passes.

test sync::once::tests::wait_for_force_to_finish has been running for a long time
error: deadlock: the evaluated program deadlocked
   --> rust\library\std\src\sys\sync\thread_parking\windows.rs:120:90
    |
120 |             c::WaitOnAddress(self.ptr(), &PARKED as *const _ as c::LPVOID, 1, c::INFINITE);
    |                                                                                          ^ the evaluated program deadlocked
    |
    = note: BACKTRACE:
    = note: inside `realstd::sys::sync::thread_parking::windows::Parker::park` at rust\library\std\src\sys\sync\thread_parking\windows.rs:120:90: 120:91    
    = note: inside `realstd::thread::Thread::park` at rust\library\std\src\thread\mod.rs:1360:18: 1360:53
    = note: inside `realstd::thread::park` at rust\library\std\src\thread\mod.rs:1103:9: 1103:25
    = note: inside `realstd::sync::mpmc::context::Context::wait_until` at rust\library\std\src\sync\mpmc\context.rs:139:17: 139:31
    = note: inside closure at rust\library\std\src\sync\mpmc\list.rs:444:27: 444:50
    = note: inside closure at rust\library\std\src\sync\mpmc\context.rs:50:13: 50:18
    = note: inside closure at rust\library\std\src\sync\mpmc\context.rs:58:31: 58:37
    = note: inside `realstd::thread::LocalKey::<core::cell::Cell<core::option::Option<realstd::sync::mpmc::context::Context>>>::try_with::<{closure@realstd::sync::mpmc::context::Context::with<{closure@realstd::sync::mpmc::list::Channel<test::event::CompletedTest>::recv::{closure#1}}, ()>::{closure#1}}, ()>` at rust\library\std\src\thread\local.rs:286:12: 286:27
    = note: inside `realstd::sync::mpmc::context::Context::with::<{closure@realstd::sync::mpmc::list::Channel<test::event::CompletedTest>::recv::{closure#1}}, ()>` at rust\library\std\src\sync\mpmc\context.rs:53:9: 62:15
    = note: inside `realstd::sync::mpmc::list::Channel::<test::event::CompletedTest>::recv` at rust\library\std\src\sync\mpmc\list.rs:434:13: 455:15        
    = note: inside `realstd::sync::mpmc::Receiver::<test::event::CompletedTest>::recv` at rust\library\std\src\sync\mpmc\mod.rs:307:43: 307:58
    = note: inside `realstd::sync::mpsc::Receiver::<test::event::CompletedTest>::recv` at rust\library\std\src\sync\mpsc\mod.rs:849:9: 849:26
    = note: inside `test::run_tests::<{closure@test::run_tests_console::{closure#2}}>` at rust\library\test\src\lib.rs:434:27: 434:36
    = note: inside `test::run_tests_console` at rust\library\test\src\console.rs:322:5: 322:70
    = note: inside `test::test_main` at rust\library\test\src\lib.rs:142:19: 142:59
    = note: inside `test::test_main_static` at rust\library\test\src\lib.rs:164:5: 164:40
    = note: inside `main`

error: deadlock: the evaluated program deadlocked
    --> library\std\src\sys\pal\windows\thread.rs:76:90
     |
76   |         let rc = unsafe { c::WaitForSingleObject(self.handle.as_raw_handle(), c::INFINITE) };
     |                                                                                          ^ the evaluated program deadlocked
     |
     = note: BACKTRACE on thread `sync::once::tests::wait_for_force_to_finish`:
     = note: inside `sys::pal::windows::thread::Thread::join` at library\std\src\sys\pal\windows\thread.rs:76:90: 76:91
...
note: inside `sync::once::tests::wait_for_force_to_finish`
    --> library\std\src\sync\once\tests.rs:115:13
     |
115  |     assert!(t2.join().is_ok());
     |             ^^^^^^^^^
...

error: deadlock: the evaluated program deadlocked
  |
  = note: the evaluated program deadlocked
  = note: (no span available)
  = note: BACKTRACE on thread `unnamed-2`:

error: deadlock: the evaluated program deadlocked
   --> library\std\src\sys\pal\windows\futex.rs:55:59
    |
55  |         c::WaitOnAddress(addr, compare_addr, size, timeout) == c::TRUE
    |                                                           ^ the evaluated program deadlocked
    |
    = note: BACKTRACE on thread `unnamed-3`:
    = note: inside `sys::pal::windows::futex::wait_on_address::<u32>` at library\std\src\sys\pal\windows\futex.rs:55:59: 55:60
note: inside `sys::pal::windows::futex::futex_wait::<u32>`
   --> library\std\src\sys\pal\windows\futex.rs:75:5
    |
75  |     wait_on_address(futex, expected, timeout) || api::get_last_error().code != c::ERROR_TIMEOUT
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside closure
   --> library\std\src\sync\once\tests.rs:106:9
    |
106 | /         O.call_once(|| {
107 | |             called = true;
108 | |         });
    | |__________^

[fmease]

sync @bors r-

[joboet]

This PR cannot have introduced the bug, we have not encountered the bug on the other platforms that use the same implementation and there recently was a rather large refactoring of the miri threading code. I think we should try this again.

@bors r+

[bors]

📌 Commit fa58d1b has been approved by joboet

It is now in the queue for this repository.

[bors]

⌛ Testing commit fa58d1b with merge 8b2338e...

[rust-log-analyzer]

The job x86_64-gnu-aux failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[bors]

💔 Test failed - checks-actions

[tgross35]

Hm, same failure. Fyi @rust-lang/miri

[saethlin]

I rebased this branch onto current master and I can reproduce the deadlock report with

MIRIFLAGS=-Zmiri-disable-isolation x miri --stage 2 library/std --target=i686-pc-windows-msvc --no-doc -- time:: sync:: thread:: env::

[saethlin]

The reproducer is just the test sync::mpsc::tests::wait_for_force_to_finish. So to be clear, that's this program:

use std::thread;
use std::panic;
use std::sync::mpsc::channel;
use std::sync::Once;

fn main() {
    static O: Once = Once::new();

    // poison the once
    let t = panic::catch_unwind(|| {
        O.call_once(|| panic!());
    });
    assert!(t.is_err());

    // make sure someone's waiting inside the once via a force
    let (tx1, rx1) = channel();
    let (tx2, rx2) = channel();
    let t1 = thread::spawn(move || {
        O.call_once_force(|p| {
            assert!(p.is_poisoned());
            tx1.send(()).unwrap();
            rx2.recv().unwrap();
        });
    });

    rx1.recv().unwrap();

    // put another waiter on the once
    let t2 = thread::spawn(|| {
        let mut called = false;
        O.call_once(|| {
            called = true;
        });
        assert!(!called);
    });

    tx2.send(()).unwrap();

    assert!(t1.join().is_ok());
    assert!(t2.join().is_ok());
}

Run using a Miri built from this branch rebased over master like so:

MIRIFLAGS="-Zmiri-disable-isolation" cargo +stage2 miri run --target=x86_64-pc-windows-msvc

(any Windows target will do)

and there recently was a rather large refactoring of the miri threading code

If the Miri scheduler were the problem, we should see problems with other platforms. But we don't. So I'm leaning towards a bug in one of the Windows shims that's being used here. The test seems to pass on Linux with --many-seeds.

[saethlin]

Oh 🤦 the fix is in the linked PR, and that is blocked on another PR, so a fix is coming at the speed of bors.

[tgross35]

🤦‍♂️ sorry about that, I missed the crosslink

[ChrisDenton]

Now that #127813 is merged, this should be good to go

@bors r+

[bors]

💡 This pull request was already approved, no need to approve it again.

• This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.
• There's another pull request that is currently being tested, blocking this pull request: Add unstable support for outputting file checksums for use in cargo #126930

[bors]

📌 Commit fa58d1b has been approved by ChrisDenton

It is now in the queue for this repository.

[bors]

⌛ Testing commit fa58d1b with merge fcc325f...

[bors]

☀️ Test successful - checks-actions
Approved by: ChrisDenton
Pushing fcc325f to master...

[rust-timer]

Finished benchmarking commit (fcc325f): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.1%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 768.932s -> 769.435s (0.07%)
Artifact size: 328.74 MiB -> 328.71 MiB (-0.01%)