net/http: TestTimeoutHandlerAndFlusher flaky on darwin-arm64-corellium

[bcmills]

The new regression test added for #34439 just flaked twice on the darwin-arm64-corellium builder:

• https://build.golang.org/log/ef6dce6843cd8802608f9895eeb21e79807e22a4
• https://build.golang.org/log/864f7b408c2ad4f67cb08581c29e926a71471138

--- FAIL: TestTimeoutHandlerAndFlusher (0.14s)
    serve_test.go:6193: Status code mismatch
        got:  418
        want: 503
    serve_test.go:6198: Body mismatch
        got:  "line1\nline2\n"
        want: "TIMED OUT\n"
FAIL
FAIL	net/http	9.476s

The time.Sleep(timeout * 2) seems suspect,¹ although I'm not certain that's the root cause of the flakes.

(Also, I have no idea why the test server thinks it is a teapot. 😅)

¹ https://testing.googleblog.com/2008/08/tott-sleeping-synchronization.html

CC @bradfitz

[bradfitz]

Let's just delete the whole test.

[odeke-em]

Thanks for the ping Bryan! That test tests out the TimeoutHandler which requires an actual timeout/sleep, not just synchronization. I think perhaps the sleep duration might have been too short. Also it reports that it is a teapot because that's the status code that was purposefully set :) I'll take a look at it when I get to the office in a few hours, am currently out running many early morning errands, thanks for the assignment.

…

On Fri, Sep 27, 2019 at 7:59 AM Bryan C. Mills ***@***.***> wrote: Assigned #34573 <#34573> to @odeke-em <https://github.com/odeke-em>. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#34573?email_source=notifications&email_token=ABFL3V32P4UY7HI5SOB4AE3QLYNUDA5CNFSM4I3HPC42YY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOT4KQ4CQ#event-2668957194>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABFL3VZZTRMFY2MC77FZCETQLYNUDANCNFSM4I3HPC4Q> .

[odeke-em]

Going nuclear SGTM but let's perhaps figure out how to catch such failures if anyone ever makes such a subtle change to TimeoutHandler.

…

On Fri, Sep 27, 2019 at 8:06 AM Brad Fitzpatrick ***@***.***> wrote: Let's just delete the whole test. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#34573?email_source=notifications&email_token=ABFL3V72VMV367PZLCKCWLDQLYOQPA5CNFSM4I3HPC42YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7ZGAEY#issuecomment-535978003>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABFL3VZ5FN2G4QUDTIWRHO3QLYOQPANCNFSM4I3HPC4Q> .

[gopherbot]

Change https://golang.org/cl/197757 mentions this issue: net/http: remove TestTimeoutHandlerAndFlusher due to flakes

[odeke-em]

I got some time in my ride and I've mailed https://go-review.googlesource.com/c/go/+/197757 to remove it.

[bcmills]

@gopherbot, please backport to Go 1.13: the flaky test was also backported.

[gopherbot]

Backport issue(s) opened: #34579 (for 1.13).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

[gopherbot]

Change https://golang.org/cl/197719 mentions this issue: [release-branch.go1.13] net/http: remove TestTimeoutHandlerAndFlusher due to flakes

[pam4]

@odeke-em, there were different failure modes:

1. handler changes w.Header(); handler calls Flush; handler returns before timeout -> client can't see header changes;
2. handler calls w.WriteHeader(someCode); handler calls Flush; handler returns before timeout -> client sees StatusOK instead of someCode;
3. handler calls Flush; timeout occurs (after Flush but before handler returns) -> client sees StatusOK instead of StatusServiceUnavailable.

(In all cases the underlying WriteHeader was called twice.)
NewTestTimeoutHandler seems useful to reproduce those conditions.
To test for #1/#2 there's no need to delay the handler, just send the timeout after the client has finished.
To test for #3 you could have the handler flush, then send the timeout, then block on a channel until the client has finished.

Anyway, if everything except Flush is already tested elsewhere, maybe just failing the test if the ResponseWriter is a Flusher is enough.
I think it's safe to assume that TimeoutHandler will never support flush: even if we managed to unblock Flush on timeout, if it does anything at all it would hinder the ability to return 503 + msg as advertised.

Thanks for fixing the issue, btw!