Skip to content

net/http/httputil: ReverseProxy FlushInterval no longer flushes headers in go1.12 #31125

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
Closed
liggitt opened this issue Mar 29, 2019 · 6 comments
Closed

net/http/httputil: ReverseProxy FlushInterval no longer flushes headers in go1.12 #31125

liggitt opened this issue Mar 29, 2019 · 6 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@liggitt
Copy link
Contributor

liggitt commented Mar 29, 2019 •
edited
Loading

What version of Go are you using (go version)?

$ go version
go version go1.12.1 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/liggitt/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/liggitt/go"
GOPROXY=""
GORACE=""
GOROOT="/Users/liggitt/.gvm/gos/go1.12.1"
GOTMPDIR=""
GOTOOLDIR="/Users/liggitt/.gvm/gos/go1.12.1/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/h8/3dzrjpfj76d93vbhc5f2r8tw00kjgb/T/go-build963504992=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

  1. Set up a reverse proxy with a flush interval.
  2. Proxied to a backend that writes and flushes headers, then streams response chunks (which can be arbitrarily delayed from the original header write/flush).

What did you expect to see?

Clients making requests to the proxy should see the headers from the backend no later than the FlushInterval

What did you see instead?

Clients making requests to the proxy receive no data until the first body chunk is written.

https://play.golang.org/p/Y8KQO8-57MR

  • go1.11 works properly
  • go1.12 hangs

The change in 5440bfc#diff-d863507a61be206d112f6e00e6d812a2L343 removed the goroutine that ensured the FlushInterval was honored while blocking in copyResponse, even prior to the first Write() call being encountered. The time.After() ticker it sets up doesn't get set up until the first Write() call is made.
This was referenced Mar 29, 2019
Closed
Closed
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/170066 mentions this issue: net/http/httputil: make ReverseProxy flush headers on FlushInterval

@simonferquel simonferquel mentioned this issue Mar 29, 2019
Merged
@thaJeztah
Copy link
Contributor

Could this be tracked for Go 1.12.2 ? (is a separate tracking issue needed for that?)

@ALTree ALTree changed the title httputil.ReverseProxy FlushInterval no longer flushes headers in go1.12 net/http/httputil: ReverseProxy FlushInterval no longer flushes headers in go1.12 Mar 29, 2019
@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 29, 2019
@ALTree
Copy link
Member

ALTree commented Mar 29, 2019

cc @bradfitz for investigation and decision regarding a possible backport

is a separate tracking issue needed for that?

In general, after the issue has been investigated, a team member will decide if it qualifies for a backport. Then a backport issue will be opened.

@PaulSD
Copy link

PaulSD commented Mar 29, 2019 •
edited
Loading

Work-around for Go 1.12:

func HttpHandler(w http.ResponseWriter, req *http.Request) {
  ...
  proxy.ServeHTTP(NewFlushResponseWriter(w), req)
}

type flushResponseWriter struct {
  http.ResponseWriter
  http.Flusher
  http.Hijacker
  http.CloseNotifier
}
func NewFlushResponseWriter(wrap http.ResponseWriter) (http.ResponseWriter) {
  f, _ := wrap.(http.Flusher)
  h, _ := wrap.(http.Hijacker)
  cn, _ := wrap.(http.CloseNotifier)
  if f == nil || h == nil || cn == nil {
    log.Printf("WARNING: http.ResponseWriter doesn't implement all flushResponseWriter interfaces, will not flush response headers")
    return wrap
  }
  return flushResponseWriter{wrap, f, h, cn}
}
func (self flushResponseWriter) WriteHeader(statusCode int) {
  self.ResponseWriter.WriteHeader(statusCode)
  self.Flusher.Flush()
}

@liggitt liggitt mentioned this issue Mar 29, 2019
Merged
@bradfitz
Copy link
Contributor

@gopherbot, please backport to Go 1.12.

@gopherbot gopherbot mentioned this issue Mar 29, 2019
Closed
@gopherbot
Copy link
Contributor

Backport issue(s) opened: #31144 (for 1.12).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@ijc ijc mentioned this issue Apr 3, 2019
Merged
3 tasks
@mfojtik mfojtik mentioned this issue Apr 12, 2019
Closed
jktomer pushed a commit to jktomer/kubernetes that referenced this issue Oct 7, 2019
Up minimum go version to 1.12.4
d34f49b 
kubernetes#77222 removed a workaround for a
Go standard library bug (golang/go#31125) that was
not fixed until that version, so we need to require it.
@jktomer jktomer mentioned this issue Oct 7, 2019
Merged
@golang golang locked and limited conversation to collaborators Mar 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@bradfitz @PaulSD @liggitt @thaJeztah @ALTree @gopherbot