search

rust-lang / jobserver-rs

Apache License 2.0
69 stars 40 forks source link

Fix helper thread termination #22

Closed Zoxc closed 7 months ago

Zoxc commented 4 years ago

This loops until the helper thread is terminated so we don't leak tokens. It also fixes https://github.com/alexcrichton/jobserver-rs/pull/20 by not looping when seeing io::ErrorKind::Interrupted if the helper thread was terminated.

alexcrichton commented 4 years ago

Thanks for the PR here, but I'm not sure I understand this change? It seems like some changes are related to the handling of panics and the other is to simply remove the 0..100 bound, but I'm not sure why that bound needs to be removed.

Zoxc commented 4 years ago

We need to loop until the helper thread is done and join it. If not we could be in a state where the helper thread has acquired a jobserver token when exiting the process, leaking said token.

I also just noticed that thread::yield_now is called while we hold a lock, which is probably not the best idea.

alexcrichton commented 4 years ago

Hm ok, I'm sorry to say but the general trend of answering questions I've seen you use that works for rust-lang/rust will not work for this project that I manage. I'd like it if PR descriptions had details as to the change being made, motivation, etc. If they don't I typically ask for more motivation or clarification, which I've tried to do here.

Your response feels very short and curt, almost implicitly assuming that I can figure out the rest if I read the code long enough. I'm unfortunately pretty busy though with other priorites and do not have the time to reverse engineer everything going on here. I would really greatly appreciate a detailed explanation as to what's going on here. Some ways to do this are:

I'm generally asking you to please describe problems in more detail. Without this understanding I am not comfortable merging new PRs like this, and while I can likely acquire the understanding myself in the limit of time, it would be extremely helpful if you were to make this a bit easier. Otherwise this will likely sit for a few weeks not actually fixing anything while I can't find time to really dig in here.