/juju

state/presence: stop watchers/pingers before TearDownTest #5543

Merged
merged 3 commits into from Jun 7, 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 6, 2016 •
Edited 1 time

Fixes LP 1588574

The panic: session already closed comes from a watcher or pinger that
is still running when the test case returns to TearDownTest. This
happens because while we always call w.Stop in a defer, w.Stop does not
actually stop the worker, it just asks it to stop.

To ensure the worker is stopped, introduce a new function assertStopped
which stops a worker and waits until it reports stopped. This method should
be used in favor of defer w.Stop() because you must ensure that all workers
that share a mongo connection (which is all of them) have stopped, and thus
are no longer using a mgo session before TearDownTest shuts down the connection.

Because this pattern of stopping and not waiting was common to both the
pinter and watcher tests, and is now resolved, we can remove the
recover in the ping() method.

(Review request: http://reviews.vapour.ws/r/4990/)
@davecheney
state/presence: stop watchers/pingers before TearDownTest 
Fixes LP 1588574

The `panic: session already closed` comes from a watcher or pinger that
is still running when the test case returns to TearDownTest. This
happens because while we always call w.Stop in a defer, w.Stop does not
actually stop the worker, it just asks it to stop.

To ensure the worker is stopped, introduce a new function `assertStopped`
which stops a worker and waits until it reports stopped.  This method should
be used in favor of defer w.Stop() because you _must_ ensure that all workers
that share a mongo connection (which is _all_ of them) have stopped, and thus
are no longer using a mgo session before `TearDownTest` shuts down the connection.

Because this pattern of stopping and not waiting was common to both the
pinter and watcher tests, and is now resolved, we can remove the
`recover` in the `ping()` method.
0ccce7a
@davecheney
Contributor

davecheney commented Jun 7, 2016

$$fixes-1588574$$
@jujubot
Contributor

jujubot commented Jun 7, 2016

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

jujubot commented Jun 7, 2016

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

davecheney commented Jun 7, 2016

$$fixes-1588574$$
@jujubot
Contributor

jujubot commented Jun 7, 2016

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

jujubot commented Jun 7, 2016

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

davecheney commented Jun 7, 2016

SHIT.

panic: Session already closed

goroutine 3555 [running]:
panic(0x14b72a0, 0xc82068c020)
    /usr/lib/go-1.6/src/runtime/panic.go:464 +0x3e6
gopkg.in/mgo%2ev2.(*Session).cluster(0xc8205faea0, 0xc82068c000)
    /home/ubuntu/juju-core_2.0-beta9/src/gopkg.in/mgo.v2/session.go:1549 +0x75
gopkg.in/mgo%2ev2.copySession(0xc8205faea0, 0xc8204bfd01, 0x2)
    /home/ubuntu/juju-core_2.0-beta9/src/gopkg.in/mgo.v2/session.go:519 +0x30
gopkg.in/mgo%2ev2.(*Session).Copy(0xc8205faea0, 0x1d46e40)
    /home/ubuntu/juju-core_2.0-beta9/src/gopkg.in/mgo.v2/session.go:1515 +0x3b
github.com/juju/juju/state/presence.(*Pinger).ping(0xc820837540, 0x0, 0x0)
    /home/ubuntu/juju-core_2.0-beta9/src/github.com/juju/juju/state/presence/presence.go:699 +0x1fc
github.com/juju/juju/state/presence.(*Pinger).loop(0xc820837540, 0x0, 0x0)
    /home/ubuntu/juju-core_2.0-beta9/src/github.com/juju/juju/state/presence/presence.go:656 +0x172
github.com/juju/juju/state/presence.(*Pinger).Start.func1(0xc820837540)
    /home/ubuntu/juju-core_2.0-beta9/src/github.com/juju/juju/state/presence/presence.go:555 +0x38
created by github.com/juju/juju/state/presence.(*Pinger).Start
    /home/ubuntu/juju-core_2.0-beta9/src/github.com/juju/juju/state/presence/presence.go:565 +0x37d
FAIL    github.com/juju/juju/apiserver/client   36.274s

I think I know where this is coming from, I'll tackle it in the morning.
@davecheney
Contributor

davecheney commented Jun 7, 2016

$$fixes-1588574$$
@jujubot
Contributor

jujubot commented Jun 7, 2016

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

jujubot commented Jun 7, 2016

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

davecheney commented Jun 7, 2016

$$fixes-1588574$$
@jujubot
Contributor

jujubot commented Jun 7, 2016

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

@jujubot jujubot merged commit dffc22e into juju:master Jun 7, 2016

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

@davecheney
state/presence: make presence workers implement worker.Worker 
Updates LP 1590161

Followup to juju/juju#5543 as requested by @fwreade

This change makes presence worker types implement worker.Worker which
allows us to use the worker.Stop(w) helper. This in turn unblocks parts
of juju/juju#5543 which had to be rolled back. See 1590161 for details.
787ca77

@davecheney davecheney referenced this pull request Jun 8, 2016

Merged

state/presence: make presence workers implement worker.Worker #5563

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

@jujubot
Merge pull request #5563 from davecheney/state-presence-assertstopped 
state/presence: make presence workers implement worker.Worker

Updates LP 1590161

Followup to juju/juju#5543 as requested by @fwreade

This change makes presence worker types implement worker.Worker which
allows us to use the worker.Stop(w) helper. This in turn unblocks parts
of juju/juju#5543 which had to be rolled back. See 1590161 for details.

(Review request: http://reviews.vapour.ws/r/5007/)
a23d99c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment