Fix the issue where Shutdown doesn't shutdown taskqueue #365
Conversation
k8s-ci-robot
added
size/M
|
/assign @bowei |
|
Is it possible to add a test for the misbehavior? |
| tq.Enqueue(cache.ExplicitKey("a")) | ||
| tq.Enqueue(cache.ExplicitKey("b")) | ||
| tq.Enqueue(cache.ExplicitKey("err")) | ||
| tq.Enqueue(cache.ExplicitKey("stop")) | ||
|
|
||
| <-doneCh | ||
| close(stopCh) |
There was a problem hiding this comment.
@bowei, the main change is to remove the stopCh from tq. Originally in order to shutdown the tq, we need to close the channel and then call tq.Shutdown(). There is no misbehavior with current implementation, only complexity. e.g. close must happen before shutdown, or it's possible that it won't be shutdown.
In terms of test, I think I can add another shutdown, to make sure no panic.
There was a problem hiding this comment.
I added L56 to test shutdown actually works.
|
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
the
lifecycle/stale
|
can you rebase this? |
Originally, PeriodicTaskQueue has 2 loops: - wait.Until managed by stopCh - for loop in worker() managed by workqueue Shutdown only exits the second loop. Failing to close stopCh before calling Shutdown will cause panic because of double close workDone channel.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: anfernee, rramkumar1 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/retest |
Originally, PeriodicTaskQueue has 2 loops:
Shutdown only exits the second loop. Failing to close stopCh before
calling Shutdown will cause panic because of double close workDone
channel.