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: make Transport ignore 408 timeout messages from server [1.12 backport] #32367

Closed
gopherbot opened this issue May 31, 2019 · 7 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Milestone

Comments

@gopherbot
Copy link

@bradfitz requested issue #32310 to be considered for backport to the next 1.12 minor release.

@gopherbot, please backport.

@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label May 31, 2019
@gopherbot gopherbot added this to the Go1.12.6 milestone May 31, 2019
@bradfitz
Copy link
Contributor

This is an unusual backport rationale: popular HTTP servers on the Internet (at least, all of Google's) changed their behavior, and without this Go logs a warning to stderr, without a way to disable it.

I suspect people will get annoyed by that, so this is somewhat preemptive. I was annoyed when I started seeing it in my logs the other day. I don't know when Google's GFEs started sending 408s, but I suspect it was recently since I started seeing the log messages.

And if we do this, maybe Go 1.11 too (#32366).

/cc @ianlancetaylor @rsc @dmitshur @bcmills

@ianlancetaylor
Copy link
Contributor

I'm fine with backporting this. It may or may not be critical, but there is no workaround.

@bradfitz
Copy link
Contributor

I suppose one workaround is people can do log.SetOutput(filteringWriter{}), but that'll get tedious and gross pretty quickly.

@dmitshur
Copy link
Contributor

dmitshur commented Jun 7, 2019

I'm going to approve this unusual backport because the workaround is quite complicated and error prone (it requires implementing a filteringWriter and not making a mistake in its implementation, or forking net/http). It's not a very serious issue itself, but it can spam log output and hide other important information from logs, which makes it more serious. I'm also taking into account that Brad and Ian are okay with it.

@dmitshur dmitshur added CherryPickApproved Used during the release process for point releases and removed CherryPickCandidate Used during the release process for point releases labels Jun 7, 2019
@gopherbot
Copy link
Author

Change https://golang.org/cl/181239 mentions this issue: [release-branch.go1.12] net/http: prevent Transport from spamming stderr on server 408 reply

@gopherbot
Copy link
Author

Closed by merging 918368e to release-branch.go1.12.

gopherbot pushed a commit that referenced this issue Jun 7, 2019
…err on server 408 reply

HTTP 408 responses now exist and are seen in the wild (e.g. from
Google's GFE), so make Go's HTTP client not spam about them when seen.
They're normal (now).

Fixes #32367
Updates #32310

Change-Id: I558eb4654960c74cf20db1902ccaae13d03310f6
Reviewed-on: https://go-review.googlesource.com/c/go/+/179457
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
(cherry picked from commit ba66d89)
Reviewed-on: https://go-review.googlesource.com/c/go/+/181239
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@andybalholm
Copy link
Contributor

popular HTTP servers on the Internet (at least, all of Google's) changed their behavior, and without this Go logs a warning to stderr, without a way to disable it.

I've been seeing this behavior for months or years. (I don't remember what servers, though.) I never thought of filing an issue for it, but I'm glad to see it fixed.

@golang golang locked and limited conversation to collaborators Jun 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Projects
None yet
Development

No branches or pull requests

5 participants