This is a writeup of an issue where dropping the helper thread can take up to 1 second on unix (discovered while investigating https://github.com/rust-lang/cargo/pull/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.
This is a writeup of an issue where dropping the helper thread can take up to 1 second on unix (discovered while investigating https://github.com/rust-lang/cargo/pull/7844). Here is a repro:
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.