Race condition in flightcontrol causing occasional test failure

[sipsma]

When running unrelated tests, I saw a one-off failure in TestCancelBoth of util/flightcontrol/flightcontrol_test.go:

=== CONT  TestCancelBoth
    flightcontrol_test.go:199:
                Error Trace:    flightcontrol_test.go:199
                Error:          Not equal:
                                expected: "bar"
                                actual  : "baz"

                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1 +1 @@
                                -bar
                                +baz
                Test:           TestCancelBoth
    flightcontrol_test.go:204:
                Error Trace:    flightcontrol_test.go:204
                Error:          Not equal:
                                expected: 3
                                actual  : 4
                Test:           TestCancelBoth
--- FAIL: TestCancelBoth (0.45s)

It doesn't reproduce very often, but I found if I cd into util/flightcontrol and run go test -run TestCancelBoth -count 250 I'll almost always get at least 1 failure (same as the one above, it's always the same errors on the same lines). Others might need to increase -count depending on the hardware being run on I suppose.

Not 100% sure but I think it happens when:

1. g.Do is called here. It returns, but the go routine it created internally is still here and hasn't been scheduled to run yet.
2. This second g.Do gets called here (with different func as callback). It gets all the way to here and thus returns the value from the previous g.Do call, even though the callback func is different.

@tonistiigi If the above is correct then I think this was introduced in this commit. What do you think the right behavior is here? Should it be that once a call to g.Do returns then any state for the key is completely removed? Or do you think that would undo the improvements to go routine contention from the previous commit?

[tonistiigi]

I think the test might not be completely valid anymore after #2195 . Flightcontrol is less deterministic now but shouldn't matter for practical cases. Or can this cause issues outside the test?

[sipsma]

Looking around at uses of flightcontrol.Do, there's a lot of calls where the callback func captures variables from the closure, so I suppose it's possible that two quick calls in succession could return the same value even when the closure variables are different.

However, now that I think about, those cases are probably bugs anyways because they should be incorporating the value of those closure variables into the key provided to flightcontrol. Otherwise there's a bug when two calls come in simultaneously with different closure variables, disregarding this case where two calls happen in succession.

Some examples of what I'm talking about (there's probably more):

1. buildkit/cache/blobs.go

   Line 56 in b2195ce

   dp, err := g.Do(ctx, sr.ID(), func(ctx context.Context) (interface{}, error) {

   (callback captures compressionType but it's not in the key)

2. buildkit/client/llb/async.go

   Line 56 in b2195ce

   _, err := as.g.Do(ctx, "", func(ctx context.Context) (interface{}, error) {

   (callback captures c but it's not in the key)

3. buildkit/executor/oci/hosts.go

   Line 24 in b2195ce

   _, err := g.Do(ctx, stateDir, func(ctx context.Context) (interface{}, error) {

   (captures hostname)

So if we are accepting that flightcontrol is less deterministic (which makes sense to me), I think that's fine because all the cases where it could cause a bug are actually bugs anyways...

Either way I guess this test should be updated since this failure is now expected. And if all those other cases are indeed bugs they should be fixed separately. Let me know if this is making sense.

[tonistiigi]

1/3 look like bugs. 2 I remember thinking about it and couldn't think of a good solution so maybe in this case it is ok if first one wins rather than some bug refactoring. It is a case that basically never happens on the client side anyway.