Merge lp:~thumper/juju-core/fix-intermittent-failure into lp:~go-bot/juju-core/trunk
Status: | Merged |
---|---|
Approved by: | Tim Penhey |
Approved revision: | no longer in the source branch. |
Merged at revision: | 1847 |
Proposed branch: | lp:~thumper/juju-core/fix-intermittent-failure |
Merge into: | lp:~go-bot/juju-core/trunk |
Diff against target: |
93 lines (+28/-19) 1 file modified
environs/sshstorage/storage_test.go (+28/-19) |
To merge this branch: | bzr merge lp:~thumper/juju-core/fix-intermittent-failure |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Juju Engineering | Pending | ||
Review via email:
|
Commit message
Fix race condition in SSHStorage test
I found an intermittent failure in the synchronization
test for the SSHStorage. It only happened once, and not
again, but I felt it was worth fixing anyway.
The race was in the flock subprocess actually starting
before the following lines in the test. The lines following
expected the flock to be taken, but since the flock was
managed by an executed command, there is a race where it
may not have started.
I broke the synchronisation test into three as it was
really testing three distinct things.
The flock helper method now waits for the flock to be taken
by incrementally reading from stdout waiting for the initial
echo to be written out prior to the sleep.
The flock cleanup is also now handled by a cleanup method.
By breaking the test up, we no longer need to manually kill the
process as part of the test.
Description of the change
Fix race condition in SSHStorage test
I found an intermittent failure in the synchronization
test for the SSHStorage. It only happened once, and not
again, but I felt it was worth fixing anyway.
The race was in the flock subprocess actually starting
before the following lines in the test. The lines following
expected the flock to be taken, but since the flock was
managed by an executed command, there is a race where it
may not have started.
I broke the synchronisation test into three as it was
really testing three distinct things.
The flock helper method now waits for the flock to be taken
by incrementally reading from stdout waiting for the initial
echo to be written out prior to the sleep.
The flock cleanup is also now handled by a cleanup method.
By breaking the test up, we no longer need to manually kill the
process as part of the test.
Reviewers: mp+186690_ code.launchpad. net,
Message:
Please take a look.
Description:
Fix race condition in SSHStorage test
I found an intermittent failure in the synchronization
test for the SSHStorage. It only happened once, and not
again, but I felt it was worth fixing anyway.
The race was in the flock subprocess actually starting
before the following lines in the test. The lines following
expected the flock to be taken, but since the flock was
managed by an executed command, there is a race where it
may not have started.
I broke the synchronisation test into three as it was
really testing three distinct things.
The flock helper method now waits for the flock to be taken
by incrementally reading from stdout waiting for the initial
echo to be written out prior to the sleep.
The flock cleanup is also now handled by a cleanup method.
By breaking the test up, we no longer need to manually kill the
process as part of the test.
https:/ /code.launchpad .net/~thumper/ juju-core/ fix-intermitten t-failure/ +merge/ 186690
(do not edit description out of merge proposal)
Please review this at https:/ /codereview. appspot. com/13799043/
Affected files (+33, -19 lines): sshstorage/ storage_ test.go
A [revision details]
M environs/
Index: [revision details] 20130919221201- urd9lbpjtto8a7p k
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-
+New revision: <email address hidden>
Index: environs/ sshstorage/ storage_ test.go sshstorage/ storage_ test.go' sshstorage/ storage_ test.go 2013-09-18 22:54:32 +0000 sshstorage/ storage_ test.go 2013-09-19 23:29:41 +0000 stor.DefaultCon sistencyStrateg y(), gc.Equals, rategy{ })
=== modified file 'environs/
--- environs/
+++ environs/
@@ -244,33 +244,42 @@
c.Assert(
utils.AttemptSt
}
-// flock is a test helper that flocks a file, eout.Seconds( )) flockBin, "--nonblock", "--close", string(mode), cmd.Start( ), gc.IsNil) func(*gc. C) {
-// executes "sleep" with the specified duration,
-// and returns the *Cmd so it can be early terminated.
-func (s *storageSuite) flock(c *gc.C, mode flockmode, lockfile string,
duration time.Duration) *os.Process {
- sleepcmd := fmt.Sprintf("sleep %vs", duration.Seconds())
+const defaultFlockTimeout = 5 * time.Second
+
+// flock is a test helper that flocks a file, executes "sleep" with the
+// specified duration, the command is terminated in the test tear down.
+func (s *storageSuite) flock(c *gc.C, mode flockmode, lockfile string) {
+ sleepcmd := fmt.Sprintf("echo started && sleep %vs",
defaultFlockTim
cmd := exec.Command(
lockfile, "-c", sleepcmd)
+ stdout, err := cmd.StdoutPipe()
+ c.Assert(err, gc.IsNil)
c.Assert(
- return cmd.Process
+ // Make sure the flock has been taken before returning by reading stdout
waiting for "started"
+ for count := len("started"); count > 0; {
+ result := make([]byte, count)
+ bytesRead, err := stdout.Read(result)
+ c.Assert(err, gc.IsNil)
+ count -= bytesRead
+ }
+ s.AddCleanup(
+ cmd.Process.Kill()
+ cmd.Process.Wait()
+ })
}
-const defaultFlockTimeout = 5 * time.Second
-
-func (s *sto...