/juju

all: don't leak pingers in tests #5564

Merged
merged 1 commit into from Jun 15, 2016

Conversation

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

davecheney commented Jun 8, 2016 •
Edited 1 time

Fixes LP 1590161

The fixtures for the many tests were starting pingers and loosing
track of them; assuming that they would die eventually when mongodb
shuts down ... which is true, except their death was not as private
an affair as previously assumed.

Fix the pinger leak by adding a cleanup action. This lets us remove the
recover in pinger.ping which previously would silently discard this
panic.

Editorial: the root cause of this problem is the mgo driver's copying
semantics -- rather than obtaining a connection to the database, you
copy a collection which is assumed to be already connected. Because this
copy operation does not return an error, the driver has forced itself
into using a panic.

Additionally the SetAgentPresence API on various 'present' things comes
with a footgun of not making clear just how important it is to keep
track of the thing that is tracking presence. The name of that API,
SetAgentPresence lulls the reader into thinking it's some kind of
boolean setter "yes, I'd like to track the presence of this thing."
There is little clue that calling that method returns a resource that
if not handled carefully will cause your program to panic randomly.
@howbazaar
Owner

howbazaar commented Jun 8, 2016

👍
@davecheney
Contributor

davecheney commented Jun 8, 2016

$$JFDI$$
@jujubot
Contributor

jujubot commented Jun 8, 2016

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

natefinch commented Jun 8, 2016

LGTM
@jujubot
Contributor

jujubot commented Jun 8, 2016

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

davecheney added a commit to davecheney/juju that referenced this pull request Jun 9, 2016

@davecheney
worker/catacomb: do not deincrement 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 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.
e1c194b

@davecheney davecheney referenced this pull request Jun 9, 2016

Merged

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

davecheney added a commit to davecheney/juju that referenced this pull request 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.
54ddc7a

davecheney added a commit to davecheney/juju that referenced this pull request 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

jujubot added a commit that referenced this pull request Jun 9, 2016

@jujubot
Merge pull request #5578 from davecheney/worker-catacomb 
worker/catacomb: do not deincrement 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 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/)
ea63958
@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/8054
@davecheney
all: don't leak pingers in tests 
Updates LP 1590161

The fixtures for the many tests were starting pingers and loosing
track of them; assuming that they would die eventually when mongodb
shuts down ... which is true, except their death was not as private
an afair as previously assumed.

Editorial: the root cause of this problem is the mgo driver's copying
semantics -- rather than obtaining a connection to the database, you
copy a collection which is assumed to be already connected. Because this
copy operation does not return an error, the driver has forced itself
into using a panic.

Additionally the SetAgentPresence API on various 'present' things comes
with a footgun of not making clear just how important it is to keep
track of the thing that is tracking presence. The name of that API,
`SetAgentPresence` lulls the reader into thinking it's some kind of
boolean setter "yes, I'd like to track the presence of this thing."
There is little clue that calling that method returns a resource that
if not handled carefully will cause your program to panic randomly.
6440a4d
cmd/jujud/agent/machine.go
if err != nil {
return err
}
// Wait for worker to finish or for us to be stopped.
- waitCh := make(chan error)
+ done := make(chan error, 1)
@howbazaar

howbazaar Jun 15, 2016

Owner

Why bother with the don channel at all? Why not just select on w.Wait() ?

@davecheney

davecheney Jun 15, 2016

Contributor

I'd love too, but it's not a channel :(

On Wed, Jun 15, 2016 at 12:21 PM, Tim Penhey notifications@github.com
wrote:

In cmd/jujud/agent/machine.go
#5564 (comment):

    if err != nil {
        return err
    }
    // Wait for worker to finish or for us to be stopped.

  •   waitCh := make(chan error)

  •   done := make(chan error, 1)

Why bother with the don channel at all? Why not just select on w.Wait() ?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/juju/juju/pull/5564/files/6440a4de17a3c6ff99a44fab67be7d3b20114cfe#r67090236,
or mute the thread
https://github.com/notifications/unsubscribe/AAAcA3DDCWJnpjOhaXB9IEDz5XRmhKJXks5qL2GogaJpZM4IwhyC
.

@davecheney
Contributor

davecheney commented Jun 15, 2016

$$JFDI$$
@jujubot
Contributor

jujubot commented Jun 15, 2016

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

jujubot commented Jun 15, 2016

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/8123
@davecheney
Contributor

davecheney commented Jun 15, 2016

$$JFDI$$
@jujubot
Contributor

jujubot commented Jun 15, 2016

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

@jujubot jujubot merged commit abf04d4 into juju:master Jun 15, 2016

@davecheney davecheney deleted the davecheney:fixedbugs/1590161 branch Jun 15, 2016

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