Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net/http: Flush in TimeoutHandler for Go1.13 is broken and might need a revert #34439

Closed
pam4 opened this issue Sep 20, 2019 · 14 comments
Closed

net/http: Flush in TimeoutHandler for Go1.13 is broken and might need a revert #34439

pam4 opened this issue Sep 20, 2019 · 14 comments
Labels
Milestone

Comments

@pam4
Copy link

@pam4 pam4 commented Sep 20, 2019

Currently (1.13) calling Flush on a ResponseWriter exposed by TimeoutHandler has some undesirable effect:

  1. it sends out default headers with a 200 OK status, ignoring whatever the caller set before (e.g. w.Header().Set(key, val) or w.WriteHeader(someOtherCode));
  2. in doing so, it prevents TimeoutHandler to respond with 503 on timeout as promised, and it produces a "superfluous response.WriteHeader call" warning;
  3. it doesn't flush anything.

https://play.golang.org/p/HmW2ETipalB

This is happening because timeoutWriter.Flush calls the underlying Flush, which in turn calls the underlying WriteHeader from a clean state: the actual header and partial body are still buffered in the wrapper (in timeoutWriter.h and timeoutWriter.wbuf).

I think TimeoutHandler supporting Flush doesn't make any sense because a TimeoutHandler doesn't know what to do until either the inner handler returns or the timeout is reached, therefore it needs to buffer the whole response until the end.

EDIT: #34198 is probably an effect.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Sep 23, 2019

@bcmills bcmills added this to the Go1.14 milestone Sep 23, 2019
@cuonglm

This comment has been minimized.

Copy link
Contributor

@cuonglm cuonglm commented Sep 23, 2019

Fair enough, maybe we can just make func (tw *timeoutWriter) Flush() becomes a no-op, so calling Flusher interface on timeout handler will do nothing.

@pam4

This comment has been minimized.

Copy link
Author

@pam4 pam4 commented Sep 23, 2019

@cuonglm, I don't understand why timeoutWriter.Flush needs to exist.
A timeoutWriter cannot flush (unless we change TimeoutHandler semantics), therefore it should not implement the Flusher interface.

Faking it just makes it impossible to test for the capability at runtime (the method doesn't even return an error). It's also a violation of the interface description: Flush sends any buffered data to the client.

Any exception should be clearly documented, if justified.

@cuonglm

This comment has been minimized.

Copy link
Contributor

@cuonglm cuonglm commented Sep 24, 2019

@pam4 I understand, but dont know if removing it break go1 compatibility or not, so better to make it do nothing, so we can prevent side effect.

@pam4

This comment has been minimized.

Copy link
Author

@pam4 pam4 commented Sep 24, 2019

but dont know if removing it break go1 compatibility or not

I don't think so. The first release to have that method was 1.13 and any existing program relying on it, is certainly not working as intended. The only "side effect" I can imagine is uncovering an already existing bug.
The problems I mentioned above seem much more serious to me, but I would certainly like to hear other opinions.

@cuonglm

This comment has been minimized.

Copy link
Contributor

@cuonglm cuonglm commented Sep 24, 2019

but dont know if removing it break go1 compatibility or not

I don't think so. The first release to have that method was 1.13 and any existing program relying on it, is certainly not working as intended. The only "side effect" I can imagine is uncovering an already existing bug.
The problems I mentioned above seem much more serious to me, but I would certainly like to hear other opinions.

So if you removed it, then all go1.13 program which rely on the interface will be broken in go1.14.

If it does not break go1 compatibility, then we can remove it . Otherwise, I think it's better to make it as a no-op, and document that timeoutWriter.Flush do nothing.

@odeke-em

This comment has been minimized.

Copy link
Member

@odeke-em odeke-em commented Sep 25, 2019

@pam4 sorry that we didn't catch this newly introduced issue earlier!
@cuonglm thank you for the no-op suggestion fix.

Perhaps we have some options from both your suggestions:
a) Making it a no-op and updating its documentation and release notes and adding a comment in its body and docs, but what else lurks when Flush is a no-op?
b) Revert it for Go1.13.X, of which Go1.13 just came out only 3 weeks ago so the blast radius won't be horrible despite it breaking compatibility briefly and then make it a point release and retroactively update the release docs as well as package docs too.

I suspect with the reasoning of being just being released 3 weeks ago, we can negotiate for a walkback as the problem is quite severe. My personal opinion is going for option b)

Kindly pinging @andybons @dmitshur @bradfitz @ianlancetaylor to help with reviewing if/how this can be done.

@odeke-em odeke-em changed the title net/http: Flush in TimeoutHandler is broken net/http: Flush in TimeoutHandler for Go1.13 and might need a revert Sep 25, 2019
@odeke-em odeke-em changed the title net/http: Flush in TimeoutHandler for Go1.13 and might need a revert net/http: Flush in TimeoutHandler for Go1.13 is broken and might need a revert Sep 25, 2019
@pam4

This comment has been minimized.

Copy link
Author

@pam4 pam4 commented Sep 25, 2019

Thanks for the reply.

IMHO saying that solution #b breaks compatibility is not entirely accurate.
timeoutWriter is unexported, so no compile-time incompatibility, and although the method exists, the interface was never actually implemented. Go 1.13 docs made a false/impossible claim and I think we should just take it back.

It's unclear to me who would benefit from option #a: any program using the method is already broken (yes, removing the method would make a single-value type assertion panic, but such panic would uncover a sure bug).
A no-op Flush would make the type assertion test useless and cause confusion if users think they are flushing but they are not.

@odeke-em odeke-em added the Soon label Sep 25, 2019
@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Sep 26, 2019

@odeke-em, let's not use Soon for this. The Soon label is only for super urgent things, like golang.org or other sites being down. We basically don't use it.

I'm fine reverting the Flusher stuff, though. That's easier than making it work properly.

@bradfitz bradfitz added NeedsFix and removed Soon labels Sep 26, 2019
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Sep 26, 2019

@gopherbot, please backport to Go 1.13: we should minimize the window during which the erroneous method is present.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 26, 2019

Backport issue(s) opened: #34560 (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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 26, 2019

Change https://golang.org/cl/197659 mentions this issue: net/http, doc/go1.13.html: revert TimeoutHandler.Flush

@odeke-em

This comment has been minimized.

Copy link
Member

@odeke-em odeke-em commented Sep 26, 2019

@odeke-em, let's not use Soon for this. The Soon label is only for super urgent things, like golang.org or other sites being down. We basically don't use it.

Oh, I had assumed that this bug was very urgent. Thank you for letting me know, Brad about its usage.
I've mailed a CL https://golang.org/cl/197659, please take a look when you've got some time.

@gopherbot gopherbot closed this in 4faf8a8 Sep 26, 2019
pokutuna added a commit to pokutuna/go that referenced this issue Oct 25, 2019
… interface.

The TimeoutHandler temporarily supported Flusher interface during development of 1.13 but it's no longer supported. This change corrects its documentation.

Fixes golang#35161
Updates golang#34439
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 25, 2019

Change https://golang.org/cl/203478 mentions this issue: net/http: fix comment TimeoutHandler no longer supports Flusher

gopherbot pushed a commit that referenced this issue Oct 25, 2019
Fixes #35161
Updates #34439

Change-Id: I978534cbb8b9fb32c115dba0066cf099c61d8ee9
GitHub-Last-Rev: d605816
GitHub-Pull-Request: #35162
Reviewed-on: https://go-review.googlesource.com/c/go/+/203478
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.