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: TimeoutHandler is incompatible with a ReverseProxy that has a negative FlushInterval #34198

Closed
hochhaus opened this issue Sep 9, 2019 · 3 comments
Milestone

Comments

@hochhaus
Copy link

@hochhaus hochhaus commented Sep 9, 2019

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

1.13.0 fails
1.12.9 works correctly

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="/home/ahochhaus/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="[elided]"
GOPROXY=""
GORACE=""
GOROOT="/usr/lib/go-1.13"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go-1.13/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build921412516=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Run the following example code under both go 1.13 and 1.12.9.

package main

import (
  "log"
  "net/http"
  "net/http/httputil"
  "net/url"
  "time"
)

func main() {
  // setup base http server                                                     
  http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
    http.Redirect(w, r, "/redirectURL", http.StatusFound)
  })
  go func() { log.Fatal(http.ListenAndServe("localhost:1024", nil)) }()

  // setup reverse proxy                                                        
  url, err := url.Parse("http://localhost:1024/")
  if err != nil {
    log.Fatal(err)
  }
  rp := httputil.NewSingleHostReverseProxy(url)
  rp.FlushInterval = -1
  rpServ := &http.Server{
    Addr:    "localhost:1025",
    Handler: http.TimeoutHandler(rp, time.Minute, "msg"),
  }
  go func() { log.Fatal(rpServ.ListenAndServe()) }()

  // send request                                                               
  c := &http.Client{
    CheckRedirect: func(req *http.Request, via []*http.Request) error {
      return http.ErrUseLastResponse
    },
  }
  resp, err := c.Get("http://localhost:1025/")
  if err != nil {
    log.Fatal(err)
  }
  if resp.StatusCode != http.StatusFound {
    log.Fatalf("expected %d. found: %d", http.StatusFound,
      resp.StatusCode)
  }
  log.Printf("success: %d", resp.StatusCode)
}

What did you expect to see?

2019/09/09 17:10:11 success: 302

What did you see instead?

2019/09/09 17:10:11 expected 302. found: 200
@hochhaus hochhaus changed the title net/http: http.TimeoutHandler is incompatible with a ReverseProxy that has a negative FlushInterval net/http: TimeoutHandler is incompatible with a ReverseProxy that has a negative FlushInterval Sep 9, 2019
@agnivade agnivade added this to the Go1.14 milestone Sep 10, 2019
@agnivade

This comment has been minimized.

Copy link
Contributor

@agnivade agnivade commented Sep 10, 2019

@pam4

This comment has been minimized.

Copy link

@pam4 pam4 commented Sep 26, 2019

@hochhaus, the incorrect return code problem should be fixed in 4faf8a8.
But please note that anything on top of a TimeoutHandler would still be unable to flush (including your ReverseProxy, regardless of your FlushInterval setting).

Leaving implementation difficulties aside, a TimeoutHandler simply cannot flush while still being able to return 503 + msg on timeout as advertised. It always buffers the whole response.

TimeoutHandler is useful if you want to limit the running time of your handler (as long as your handler aborts on write errors), but note that the actual sending of the data can still take any amount of time (unless the global Server.WriteTimeout is set).

Unfortunately there is still no way to set different write timeouts for different endpoints, nor to (re)set the timeout from a running handler, see #16100.

Also note that your repro program is racy; you should at least introduce a delay before the Get. And you shouldn't be able to observe the return code problem with Go 1.12.9 because timeoutWriter.Flush was not present yet.

@hochhaus

This comment has been minimized.

Copy link
Author

@hochhaus hochhaus commented Sep 27, 2019

Thanks @pam4. I'll update my code accordingly.

@hochhaus hochhaus closed this Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.