crypto/tls: add TestClientHandshakeContextCancellation back after fixing flake

[katiehockman]

Related to #45084 and #32406

We had to remove the test in 2f3db22 since it was flaky, so we should fix it and add it back.

@johanbrandhorst I've gone ahead and assigned this to you since you wrote the original code and removed the test, but let us know if you don't have time to fix it and we can assign it to someone on our team.

/cc @FiloSottile @rolandshoemaker

[johanbrandhorst]

Thanks for the follow-up Katie, I should be able to find the time for this, but I'll let you know if not.

[johanbrandhorst]

I'm having some problems reproducing this failure on my machine, any thoughts on the environment that triggers this?

$ go test -run '^TestClientHandshakeContextCancellation$' ./crypto/tls
ok      crypto/tls      0.019s
$ go test -run '^TestClientHandshakeContextCancellation$' -count=100 ./crypto/tls
ok      crypto/tls      0.039s
$ go test -run '^TestClientHandshakeContextCancellation$' -count=1000 ./crypto/tls
ok      crypto/tls      0.215s
$ go test -run '^TestClientHandshakeContextCancellation$' -count=100000 ./crypto/tls
ok      crypto/tls      19.952s
$ go test -run '^TestClientHandshakeContextCancellation$' -race -count=1 ./crypto/tls
ok      crypto/tls      0.071s
$ go test -race -count=1 ./crypto/tls/...
ok      crypto/tls      5.215s
$ go test -race -count=10 ./crypto/tls/...
ok      crypto/tls      31.854s
$ go test -count=10 ./crypto/tls/...
ok      crypto/tls      14.045s

[katiehockman]

Here's one of the builder failures, which describes the environment some: https://build.golang.org/log/6e8d66419f4cf08633e84e3c03777ccbf3f41e54

Based on the logs @bcmills provided in the issue, it wasn't affecting darwin architectures, so if you're running those tests from a Mac, that might be why you aren't seeing it. Maybe try a Linux machine, if you have one.

[johanbrandhorst]

Thanks for the link, I'll have a look. I'm running it on linux/amd64. Maybe I'll just have to run the whole testsuite. I assumed it might be contention on the localPipe but even running the whole crypto/tls suite didn't seem to trigger it. I'll play around with it a bit more, but if someone else finds a good way to reproduce the error, or better yet, have a good idea about what the problem could be, let me know!

[gopherbot]

Change https://golang.org/cl/305250 mentions this issue: crypto/tls: reintroduce simplified cancellation test

[millerresearch]

As mentioned on #45299, I can reproduce this on a Raspberry Pi (plan9-arm) pretty reliably. It goes away if a 50 millisecond delay is inserted between the conn.readClientHello and cancel. I'm looking for the race now.

[millerresearch]

Another clue: on linux-amd64 I can induce a race by inserting a delay into the interrupter goroutine in the (*Conn).handshakeContext function like so:

  go func() {
    <-time.After(50*time.Millisecond)
    select {
    case <-handshakeCtx.Done():

On a heavily loaded system, this sort of delay could occur naturally.

[johanbrandhorst]

The implication then is that the test may fail if the interrupter goroutine hasn't had a chance to start before the connection failed? That would explain the flake, but how do we fix it? The test is failing because it's not seeing the context.Canceled error, so we could accept either context.Canceled or io.EOF as errors, but how does that compromise the value of the test?

[millerresearch]

Is io.EOF actually an acceptable result when the server cancels the handshake? If not, then it's not the test that needs correcting, but the underlying implementation's vulnerability to races.