/juju

worker/catacomb: do not deincrement catacomb.wg until worker has stopped #5578

Merged
merged 2 commits into from Jun 9, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@davecheney @jujubot
@davecheney
Contributor

davecheney commented Jun 9, 2016 •
Edited 1 time

This change is a prerequisite of #5564, LP 1590161

Prior to this change, with #5564 applied, the cmd/jujud/agent tests
would reliably cause a 'session already closed' panic because the
workers owned by the catacomb had not yet stopped.

This change alters the catacomb.add logic to use the worker.Stop helper
that ensures the worker has stopped before deincrementing
catacomb.wg. In turn, assuming that the catacomb's owner waits until
its tomb is Dead, this ensures that the catacomb is not considered dead
until all its workers are fully stopped.

With this change in place, the cmd/jujud/agent tests pass reliably.

(Review request: http://reviews.vapour.ws/r/5022/)
@davecheney
Contributor

davecheney commented Jun 9, 2016

$$JFDI$$
@jujubot
Contributor

jujubot commented Jun 9, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju
@jujubot
Contributor

jujubot commented Jun 9, 2016

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/8051

davecheney added some commits Jun 9, 2016

@davecheney
worker/catacomb: do not decrement catacomb.wg until worker has stopped 
This change is a prerequisite of #5564, LP 1590161

Prior to this change, with #5564 applied, the cmd/jujud/agent tests
would reliably cause a 'session already closed' panic because the
workers owned by the catacomb had not yet stopped.

This change alters the catacomb.add logic to use the worker.Stop helper
that ensures the worker has stopped before decrementing catacomb.wg.
In turn, assuming that the catacomb's owner waits until its tomb is Dead,
this ensures that the catacomb is not considered dead until all its
workers are fully stopped.

With this change in place, the cmd/jujud/agent tests pass reliably.
5a27c67
@davecheney
worker/catacomb: fix LP 1590645 
Fix a logical race where the catacomb may not report the final error
from a worker. The fix is to wait for _both_ goroutines to exit before
letting the catacomb return.
182d0b1
@davecheney
Contributor

davecheney commented Jun 9, 2016

$$JFDI$$
@jujubot
Contributor

jujubot commented Jun 9, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit ea63958 into juju:master Jun 9, 2016

@davecheney davecheney deleted the davecheney:worker-catacomb branch Jun 9, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment