Move scheduler/threadGroup into common-init instead of per-app #12266
Conversation
|
Needs an 0.16 tag. |
TheBlueMatt
force-pushed
the
2018-01-common-init
branch
from
bc0df97
to
935f287
Compare
src/init.cpp
Outdated
| @@ -155,7 +155,10 @@ class CCoinsViewErrorCatcher final : public CCoinsViewBacked | |||
| static std::unique_ptr<CCoinsViewErrorCatcher> pcoinscatcher; | |||
| static std::unique_ptr<ECCVerifyHandle> globalVerifyHandle; | |||
|
|
|||
| void Interrupt(boost::thread_group& threadGroup) | |||
| boost::thread_group threadGroup; | |||
src/init.cpp
Outdated
| @@ -155,7 +155,10 @@ class CCoinsViewErrorCatcher final : public CCoinsViewBacked | |||
| static std::unique_ptr<CCoinsViewErrorCatcher> pcoinscatcher; | |||
| static std::unique_ptr<ECCVerifyHandle> globalVerifyHandle; | |||
|
|
|||
| void Interrupt(boost::thread_group& threadGroup) | |||
| boost::thread_group threadGroup; | |||
| CScheduler scheduler; | |||
This resolves bitcoin#12229 which pointed out a shutdown deadlock due to scheduler/checkqueue having been shut down while network message processing is still running.
TheBlueMatt
force-pushed
the
2018-01-common-init
branch
from
935f287
to
082a61c
Compare
|
Since the scheduler is going to need a proper Interrupt/Shutdown before dropping the threadGroup anyway, can we just take this opportunity to create those? Then we could just skip the threadGroup.interrupt_all(), and the .join_all() would finish immediately. That would be much easier to review, IMO, because it makes the scheduler's teardown procedure explicit. |
Right - the longer term plan is to rid of 'threadgroup' completely, by giving each 'module' its own Interrupt/Shutdown. But more often than not, changes to shutdown introduced new problems. I think this is very risky to do this last minute for 0.16. Is there a more minimalistic, easy to review fix for #12229 that we can do for 0.16, then leave the refactors for 0.17? |
|
Given that threadGroup is only used for scheduler and checkqueue, I'd strongly object to this being more than a rather-minimalistic change. The "obvious, minimalistic" change is to keep the scheduler running until after CConnman has been Stop()ed, which this does, the only additional thing is it keeps the checkqueue running, which is a rather trivial change. |
Agree. Is there a way I can reproduce the issue? |
|
I discussed with @TheBlueMatt on IRC and now I think this is very straightforward - there's not much left using the thread group (only the scheduler and checkqueue), and anything that removes the global thread group will also need the changes to remove the arguments. So it's a step forward in that regard. So utACK 082a61c |
|
I'm not too familiar with the thread management but this looks straightforward enough for me to at least give a concept ACK / tentative utACK 082a61c |
…per-app 082a61c Move scheduler/threadGroup into common-init instead of per-app (Matt Corallo) Pull request description: This resolves #12229 which pointed out a shutdown deadlock due to scheduler/checkqueue having been shut down while network message processing is still running. Tree-SHA512: 0c0a76113996b164b0610d3b8c40b396f3e384d165bf098768e31fe3701b00763d0d810ef24702387e2e936fefb9fb900a6225f7417bb0175b585f365d542660
|
posthumous utACK |
…ead of per-app 082a61c Move scheduler/threadGroup into common-init instead of per-app (Matt Corallo) Pull request description: This resolves bitcoin#12229 which pointed out a shutdown deadlock due to scheduler/checkqueue having been shut down while network message processing is still running. Tree-SHA512: 0c0a76113996b164b0610d3b8c40b396f3e384d165bf098768e31fe3701b00763d0d810ef24702387e2e936fefb9fb900a6225f7417bb0175b585f365d542660
…nit instead of per-app" This reverts commit 3eff18c.
…ead of per-app 082a61c Move scheduler/threadGroup into common-init instead of per-app (Matt Corallo) Pull request description: This resolves bitcoin#12229 which pointed out a shutdown deadlock due to scheduler/checkqueue having been shut down while network message processing is still running. Tree-SHA512: 0c0a76113996b164b0610d3b8c40b396f3e384d165bf098768e31fe3701b00763d0d810ef24702387e2e936fefb9fb900a6225f7417bb0175b585f365d542660
…nit instead of per-app" This reverts commit 3eff18c.
…ead of per-app 082a61c Move scheduler/threadGroup into common-init instead of per-app (Matt Corallo) Pull request description: This resolves bitcoin#12229 which pointed out a shutdown deadlock due to scheduler/checkqueue having been shut down while network message processing is still running. Tree-SHA512: 0c0a76113996b164b0610d3b8c40b396f3e384d165bf098768e31fe3701b00763d0d810ef24702387e2e936fefb9fb900a6225f7417bb0175b585f365d542660
…ead of per-app 082a61c Move scheduler/threadGroup into common-init instead of per-app (Matt Corallo) Pull request description: This resolves bitcoin#12229 which pointed out a shutdown deadlock due to scheduler/checkqueue having been shut down while network message processing is still running. Tree-SHA512: 0c0a76113996b164b0610d3b8c40b396f3e384d165bf098768e31fe3701b00763d0d810ef24702387e2e936fefb9fb900a6225f7417bb0175b585f365d542660
Backport bitcoin#12266 and bitcoin#13894 to fix shutdown issues
…ead of per-app 082a61c Move scheduler/threadGroup into common-init instead of per-app (Matt Corallo) Pull request description: This resolves bitcoin#12229 which pointed out a shutdown deadlock due to scheduler/checkqueue having been shut down while network message processing is still running. Tree-SHA512: 0c0a76113996b164b0610d3b8c40b396f3e384d165bf098768e31fe3701b00763d0d810ef24702387e2e936fefb9fb900a6225f7417bb0175b585f365d542660
This resolves #12229 which pointed out a shutdown deadlock due to
scheduler/checkqueue having been shut down while network message
processing is still running.