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
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@pam4
Copy link

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
Copy link
Contributor

bcmills commented Sep 23, 2019

CC @cuonglm @bradfitz

See CL 154383 / #29193.

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 23, 2019
@bcmills bcmills added this to the Go1.14 milestone Sep 23, 2019
@cuonglm
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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
Copy link
Member

@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 added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Sep 25, 2019
@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
Copy link
Author

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 This needs to be done soon. (regressions, serious bugs, outages) label Sep 25, 2019
@bradfitz
Copy link
Contributor

@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 The path to resolution is known, but the work has not been done. and removed Soon This needs to be done soon. (regressions, serious bugs, outages) labels Sep 26, 2019
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 26, 2019
@bcmills
Copy link
Contributor

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
Copy link

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
Copy link

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

@odeke-em
Copy link
Member

@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.

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
Copy link

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>
@golang golang locked and limited conversation to collaborators Oct 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants