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.

Sign up for GitHub

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

Jump to bottom

net/http: ResponseWriter panics in WriteHeaders that were formerly ignored #23010

Closed
rsc opened this issue Dec 6, 2017 · 5 comments
Closed

net/http: ResponseWriter panics in WriteHeaders that were formerly ignored #23010

rsc opened this issue Dec 6, 2017 · 5 comments
Labels
FrozenDueToAge release-blocker
Milestone
Go1.10

Comments

@rsc
Copy link
Contributor

rsc commented Dec 6, 2017

See comments on CL 80077.

I am worried about (accidental) sequences like:

w.Write([]byte("whatever"))
w.WriteHeader(badCode)

or

w.Hijack()
w.WriteHeader(badCode)

Those WriteHeader calls were formerly reported in a logf as out of sequence but otherwise ignored. They should probably continue to be logf+no-op instead of causing a panic.
@rsc rsc added this to the Go1.10 milestone Dec 6, 2017
@rsc
Copy link
Contributor Author

rsc commented Dec 6, 2017

It's also not obvious why mapping to a 500 or some other internal error wouldn't be better than panic.

@bradfitz
Copy link
Contributor

bradfitz commented Dec 6, 2017

See some discussion about this on #22880.

The point is to be consistent between HTTP/1 and HTTP/2. The hack we did for HTTP/1 was never right anyway, had no HTTP/2 analogue, and only hid real problems from users. (e.g. #22880 (comment))

People won't notice a 500. I'd like to keep the panic, at least for the betas and see if it trips up anybody whose code wasn't broken.

@bradfitz
Copy link
Contributor

bradfitz commented Dec 7, 2017

I'd be fine changing this to code 500 too as long as it's accompanied by stderr log spam so people can fix their code.

But I'd rather do that after a beta or two if it's actually necessary.

@gopherbot
Copy link

Change https://golang.org/cl/86255 mentions this issue: http2: don't check WriteHeader status if we've already sent the header

@bradfitz bradfitz self-assigned this Jan 4, 2018
@gopherbot
Copy link

Change https://golang.org/cl/86275 mentions this issue: net/http: don't validate WriteHeader code if header's already been sent

gopherbot pushed a commit to golang/net that referenced this issue Jan 5, 2018
@bradfitz
http2: don't check WriteHeader status if we've already sent the header
42fe2e1 
Tests will be in net/http in a separate CL.

Updates golang/go#23010

Change-Id: I91a6875b9a59b33a3c669e73dd6632ac523eb9f6
Reviewed-on: https://go-review.googlesource.com/86255
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018
@bradfitz
http2: don't check WriteHeader status if we've already sent the header
d372865 
Tests will be in net/http in a separate CL.

Updates golang/go#23010

Change-Id: I91a6875b9a59b33a3c669e73dd6632ac523eb9f6
Reviewed-on: https://go-review.googlesource.com/86255
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@yargevad yargevad mentioned this issue Aug 27, 2018
Closed
@golang golang locked and limited conversation to collaborators Jan 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge release-blocker
Projects
None yet
Milestone
Go1.10
Development

No branches or pull requests
3 participants
@bradfitz @rsc @gopherbot