TestRunBacklogFailedContinuesDiffApp is racy

[Michael Hudson-Doyle]

TestRunBacklogFailedContinuesDiffApp fails with Go 1.5 like this:

kindpool_test.go:544:
    // Everything up to here was just set-up.
    //
    // What we're checking for is that, if a helper launch fails, the
    // next one in the backlog is picked up.
    c.Assert(takeNext(ch, c).Input.App, Equals, app1)
... obtained *click.AppId = &click.AppId{Package:"com.example.test", Application:"test-app-2", Version:"", Click:true, original:"com.example.test_test-app-2"} ("com.example.test_test-app-2")
... expected *click.AppId = &click.AppId{Package:"com.example.test", Application:"test-app-1", Version:"", Click:true, original:"com.example.test_test-app-1"} ("com.example.test_test-app-1")

This is a race. It's a bit confusing -- and I don't really understand this code -- but it's something like this:

TestRunBacklogFailedContinuesDiffApp calls s.pool.Start(). This starts pool.loop() in it's own goroutine (the "loop goroutine").

The test causes the "in, ok := <-pool.chIn" case of the select in loop to be entered twice: the first time the loop goroutine calls handleOne and the second time it just puts the input in backlog.

The test then calls "go s.fakeLauncher.done("0")". This winds up calling pool.OneDone in a new goroutine. OneDone does two things relevant to this bug: it calls "pool.chDone <- args.Input.App" and it calls "pool.chOut <- res". The problem is that the loop goroutine is sitting in a select on pool.chDone and pool.chIn, and if it gets to run before OneDone does its second send, it will push the failure for app2 into chOut before OneDone pushes the result for app1. Hence the race above!

It's possible to make this fail on 1.4 by putting a small sleep after the send to chDone.

[Michael Hudson-Doyle (mwhudson)]

There is a similar ish race in TestRunNthAppToBacklog: this time, the channel read on the other end of s.pool.Run("fake", input3) races with s.fakeLauncher.done("0"). If the latter completes first, no job gets onto the backlog and the test fails.

[Samuele Pedroni (pedronis)]

I have a branch that should fix those,

as I expected I had to hook a bit in the code but not too bad,

otoh TestRunBacklogFailedContinuesDiffApp was actually doing a bit of harmless non-sense, not sure why it was doing ...done("2") there for example