Skip to content

Fix helper thread termination #22

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

Closed
wants to merge 2 commits into from
Closed

Conversation

Zoxc
Copy link

@Zoxc Zoxc commented Jan 17, 2020

This loops until the helper thread is terminated so we don't leak tokens. It also fixes #20 by not looping when seeing io::ErrorKind::Interrupted if the helper thread was terminated.

@alexcrichton
Copy link
Member

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
Copy link
Author

Zoxc commented Jan 21, 2020

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
Copy link
Member

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:

  • Longer than sentence fragment commit messages
  • Longer than two-sentence PR descriptions
  • More detailed answers to follow-up questions that I ask

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.

@Zoxc Zoxc closed this Jan 19, 2024
@Zoxc Zoxc deleted the join-helper-thread branch January 19, 2024 05:33
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

Successfully merging this pull request may close these issues.

2 participants