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: ResponseWriter panics in WriteHeaders that were formerly ignored #23010

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

Comments

Projects
None yet
3 participants
@rsc
Copy link
Contributor

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 rsc added the release-blocker label Dec 6, 2017

@rsc

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link

commented Jan 4, 2018

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

This comment has been minimized.

Copy link

commented Jan 4, 2018

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

http2: don't check WriteHeader status if we've already sent the header
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>

@gopherbot gopherbot closed this in 596e3d9 Jan 5, 2018

c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018

http2: don't check WriteHeader status if we've already sent the header
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>

@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.
You can’t perform that action at this time.