fix unintended deadlock on key prefixes

[glycerine]

After winning a campaign, the call to waitDeletes() was missing a
slash after the key prefix. The result was deadlock due to
waiting on wrong keys; if a prior key exists that has the new key
as a prefix.

Fixes #6278

[glycerine]

@xiang90, @heyitsanthony
Could you take a look.

The CI tests seem to fail for "package not found" reasons I don't understand, and seem unrelated to the patch.

[xiang90]

@glycerine Sorry for the inconvenience. The current master is broken due to a testing issue. We first need to get that first tomorrow.

[glycerine]

@xiang90 -- no worries. As for the patch, I was wondering -- would it be better to push the change into the function waitDeletes(), so that any other future caller wouldn't make the same mistake? I didn't see any other uses of waitDeletes(), but I also can't tell if there are other expectations/legacy users I'm unaware of.

[xiang90]

@glycerine

I think we can actually add / or any other hard to conflict char to the end of the keyPrefix. So users will not accidentally pollute the key space used for election or locking. Right now, not only waitDeletes watches on raw prefix but also some other functions (https://github.com/coreos/etcd/blob/master/clientv3/concurrency/election.go#L147-L158) too.

I even think we probably should add prefixes like /lock/xy/. It can avoid conflicts and we can support operations like list locks, elections in the future. But that can break existing users :(.

It would be helpful if you can also write a test case for this in https://github.com/coreos/etcd/blob/master/integration/v3_election_test.go.

With all that being side, @heyitsanthony knows this part better than me and might provide better opinions.

[heyitsanthony]

@glycerine thanks for the patch. I agree with @xiang90 that it'd be cleaner to add the "/" to the prefix in NewMutex/NewElection constructors up front and replacing the Sprintf("%s/%s"...) with Sprintf("%s%s").

Also please fix the commit title or CI won't accept it (the error is Expected commit title format '<package>{", "<package>}: <description>').

@xiang90 I feel like prefixing with /lock/xy breaks etcd3's clean keyspace guarantee since it reserves /lock/ from the start as having some additional meaning.

[glycerine]

Done:

1. title in commit changed to include package clientv3/concurrency.
2. test added to integration/v3_election_test.go.
3. slash added to prefix during NewElection and NewMutex construction, per suggestion.

[glycerine]

TravisCI passes. Semaphoreci points out that TestTxnReadRetry and TestWatchReconnRequest are flakey tests, independent of this change.

@xiang90, @heyitsanthony: ready to merge?

[heyitsanthony]

This test is more complicated than it needs to be. I think this should trigger the old, broken behavior with a lot less code:

clus := NewClusterV3(t, &ClusterConfig{Size : 1})
defer clus.Terminate(t)

cli := clus.RandClient()
if _, err := cli.Put(context.TODO(), "testa"); err != nil {
    t.Fatal(err)
}
s, serr :=  concurrency.NewSession(cli)
if serr != nil {
    return serr
}
e := concurrency.NewElection(s, "test")
ctx, cancel := context.WithTimeout(context.TODO(), 5*time.Second)
err := e.Campaign(ctx, "abc")
cancel()
if err != nil {
    t.Fatal(err)
}

[xiang90]

@glycerine Can you rebase with the master? We just fixed the CI issue on master. Sorry about the inconvenience.

[glycerine]

@heyitsanthony Cool. This is really a pretty small change and obviously you know the code much better than me. Why do you go ahead and merge your test version and fix per your preference.

[heyitsanthony]

@glycerine are you abandoning this PR or what? Just update the test and I can merge this...?

[glycerine]

@heyitsanthony :Okay, rebased and updated test.

[heyitsanthony]

lgtm

[xiang90]

@glycerine Thanks for the contribution!

[glycerine]

cool -- @heyitsanthony one quick question for future reference so I understand how these tests and context interact -- why is cancel() called after the Campaign() call? -- the Campaign() has already succeeded or timed-out by that point, so why is it needed?

e := concurrency.NewElection(s, "test")
ctx, cancel := context.WithTimeout(context.TODO(), 5*time.Second)
err := e.Campaign(ctx, "abc") // pauses for 5 seconds without the bug fix
cancel() // what does this do?
if err != nil {

[xiang90]

@glycerine That is the intended behavior for releasing the resource associated with the context. See the doc in context pkg: https://godoc.org/golang.org/x/net/context#WithCancel