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

Decrement thread count on thread exit. #1562

Merged
merged 3 commits into from
Sep 19, 2019

Conversation

jothan
Copy link
Contributor

@jothan jothan commented Sep 17, 2019

I hit a problem that after 10 seconds idle, no blocking tasks would
run anymore as all blocking threads exited and the Pool was convinced
that there were still runners available.

Nowhere in the code is num_th decremented. Please make sure the num_idle decrement is correct at that point in the code.

Motivation

Solution

I hit a problem that after 10 seconds idle, no blocking tasks would
run anymore as all blocking threads exited and the Pool was convinced
that there were still runners available.
@jothan
Copy link
Contributor Author

jothan commented Sep 17, 2019

It looks like I duped #1560, but that patch does not decrement the total thread count.

@jothan
Copy link
Contributor Author

jothan commented Sep 17, 2019

I don't know what happened, but I managed to hit a decrement that underflowed num_idle to 2^32-1 with this patch applied.

A notify at the wrong time can otherwise get us to underflow.
@jothan
Copy link
Contributor Author

jothan commented Sep 17, 2019

@carllerche Thanks for the review !

@AnickaBurova
Copy link

I just noticed another issue:
shared.num_idle += 1; is incrementing,
but when the task is executed, we continue 'outer'; and the increment will happen again. And we have one thread but 2 idles!
I think num_idle should be decremented there as well. Please check.

@jothan
Copy link
Contributor Author

jothan commented Sep 18, 2019

@AnickaBurova for the case you are mentionning, if I am not mistaken, the idle count should be decremented by 1 for every wakeup signaled. Have you hit a case in practice where num_idle > num_th ?

edit: Have you hit a case in practice where num_idle > num_th with this patch applied ?

@carllerche
Copy link
Member

Merging master should fix the clippy failure (#1569).

@carllerche
Copy link
Member

I took the liberty of merging master to get this fix in sooner 👍

@carllerche carllerche merged commit 22a3b10 into tokio-rs:master Sep 19, 2019
@jothan jothan deleted the fix-blocking-bookeeping branch September 20, 2019 11:41
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.

None yet

3 participants