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

Helper thread drop can sometimes take 1 second #23

Closed
ehuss opened this issue Jan 29, 2020 · 1 comment
Closed

Helper thread drop can sometimes take 1 second #23

ehuss opened this issue Jan 29, 2020 · 1 comment

Comments

@ehuss
Copy link

ehuss commented Jan 29, 2020

This is a writeup of an issue where dropping the helper thread can take up to 1 second on unix (discovered while investigating rust-lang/cargo#7844). Here is a repro:

use std::sync::*;
use std::sync::atomic::*;

fn main() {
    let client = jobserver::Client::new(4).unwrap();
    static COUNT: AtomicU32 = AtomicU32::new(0);
    let tokens = Arc::new(Mutex::new(Vec::new()));
    let helper = client.into_helper_thread(move |token| {
        tokens.lock().unwrap().push(token);
        COUNT.fetch_add(1, Ordering::SeqCst);
    }).unwrap();

    // Request more tokens than what are available.
    for _ in 0..5 {
        helper.request_token();
    }
    // Wait for at least some of the requests to finish.
    while COUNT.load(Ordering::SeqCst) < 3 {
        std::thread::yield_now();
    }
    // Drop helper
    let t = std::time::Instant::now();
    drop(helper);
    let d = t.elapsed();
    if d.as_secs_f64() > 0.5 {
        println!("took {:?} seconds!!!!!!!!!!!!!", d);
    }
}

Running this usually (not always) takes about 1 second to drop.

The gist is that if you request too many tokens, and then try to drop the helper thread, it is unable to stop it. The reason is:

  • for_each_request is stuck on this line, waiting for poll.
  • Helper::join attempts to wake it up. But the acquire loop does not check why it is being interrupted, and simply tries to go back to polling.

It looks like #22 attempts to address this (and this test seems to pass with that change), but I have not reviewed that PR in detail.

@alexcrichton
Copy link
Member

Oh dear this is embarassing. I think this was an accidental bug from #20 which snuck in by accident!

alexcrichton added a commit to alexcrichton/rust that referenced this issue Jan 30, 2020
Brings in a fix for rust-lang/jobserver-rs#23 which could cause Cargo
to unnecessarily hang in some situations.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jan 30, 2020
…lan-DPC

Update jobserver crate to 0.1.21

Brings in a fix for rust-lang/jobserver-rs#23 which could cause Cargo
to unnecessarily hang in some situations.
bors added a commit to rust-lang/rust that referenced this issue Jan 31, 2020
Update jobserver crate to 0.1.21

Brings in a fix for rust-lang/jobserver-rs#23 which could cause Cargo
to unnecessarily hang in some situations.
Mark-Simulacrum pushed a commit to Mark-Simulacrum/rust that referenced this issue Feb 6, 2020
Brings in a fix for rust-lang/jobserver-rs#23 which could cause Cargo
to unnecessarily hang in some situations.
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

No branches or pull requests

2 participants