net/http: Server negative timeouts are accepted and break silently

[belimawr]

I am not completely sure whether it is a bug, an expected behaviour or just a lack of documentation, however, I believe that this behaviour should at least be documented, ideally have the application explicitly failing.

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

$ go version
go version go1.14.3 linux/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
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/onefootball/.cache/go-build"
GOENV="/home/onefootball/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY="github.com/motain/*"
GONOSUMDB="github.com/motain/*"
GOOS="linux"
GOPATH="/home/onefootball/go"
GOPRIVATE="github.com/motain/*"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/onefootball/sdk/go1.14.3"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/onefootball/sdk/go1.14.3/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/onefootball/test/go.mod"
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-build006613611=/tmp/go-build -gno-record-gcc-switches"
GOROOT/bin/go version: go version go1.14.3 linux/amd64
GOROOT/bin/go tool compile -V: compile version go1.14.3
uname -sr: Linux 5.6.13-arch1-1
/usr/lib/libc.so.6: GNU C Library (GNU libc) stable release version 2.31.

What did you do?

Started an HTTP server with negative values for WriteTimeout and ReadTimeout, then executed requests to this HTTP server.

Here is a super simple code that reproduces the issue:

package main

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

func main() {
	server := &http.Server{
		WriteTimeout: -time.Second,
		ReadTimeout:  -time.Second,
		Addr:         ":3000",
	}

	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		log.Print("Request:", r.URL.String())
	})

	server.Handler = handler

	log.Print("starting HTTP server on port: 3000")
	panic(server.ListenAndServe())
}

What did you expect to see?

One of:

• An error log or panic on HTTP server start up
• Any sort of error log when accepting/rejecting requests
• Documentation on net/http.Server explaining the behaviour when the timeout is negative

What did you see instead?

Nothing on application side, for any caller the only kind of error we see is a "connection reset by peer".

onefootball@millennium-falcon ~ % curl localhost:3000/
curl: (56) Recv failure: Connection reset by peer

[cagedmantis]

/cc @bradfitz @bcmills

[odeke-em]

For an isolated and runnable repro, please see https://play.golang.org/p/uD5-UNDhTyN or inlined below

package main

import (
	"io/ioutil"
	"net/http"
	"net/http/httptest"
	"time"
)

func main() {
	cst := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		w.Write([]byte("Hello, issue!"))
	}))
	cst.Config.WriteTimeout = -time.Second
	cst.Config.ReadTimeout = -time.Second
	cst.Start()
	defer cst.Close()

	for i := 0; i < 10; i++ {
		res, err := cst.Client().Get(cst.URL)
		if err != nil {
			panic(err)
		}
		_, _ = ioutil.ReadAll(res.Body)
		res.Body.Close()
	}
}

[jharshman]

I'm not sure if setting a negative value for those fields has any practical use. Perhaps testing?

I could see documentation being enough for this but it's also a small code change to not add negative values to the timeouts.

[belimawr]

I don't see any use of negative values there either. For testing very small, positive values could achieve the same result.

I believe not accepting negative values (or setting them to zero) would make http.Server safer to use. However I wonder how that could affect existing programs and impact the go compatibility promise.

[gopherbot]

Change https://golang.org/cl/235437 mentions this issue: net/http: add to deadlines only when positive

[neild]

I believe the current behavior is consistent with the documentation: A negative timeout gives less than no time in which to perform the operation. This isn't useful, but a 1ns timeout isn't really useful either.

I don't think there's any need to change the package behavior or documentation with regard to negative timeouts.

I do note that there is no documentation of what a zero WriteTimeout means. We should explicitly document that this indicates no timeout.

[bradfitz]

Sometimes we use negative values to mean "disabled, no timeout" when 0 means "pick some default timeout value for me".

I'm fine with making negative values an error or disabled. I agree we should say what 0 means for ReadTimeout.

[belimawr]

Sometimes we use negative values to mean "disabled, no timeout" when 0 means "pick some default timeout value for me".

I'm fine with making negative values an error or disabled. I agree we should say what 0 means for ReadTimeout.

I believe we should say what 0 means for both of them (ReadTimeout and WriteTimeout).

Making negative values an error or disabled are both fine for me. However, I believe that making negative values an error is "safer" in the sense that it's easier to spot errors. What lead me to open this issue was an overflow of time.Duration (int64) that make the timeouts negative without nobody noticing it.

[bcmills]

@belimawr given that negative values already have a well-defined behavior, that behavior arguably cannot be changed due to the Go 1 compatibility policy.

We can certainly improve the documentation, though.

[bcmills]

What lead me to open this issue was an overflow of time.Duration (int64) that make the timeouts negative without nobody noticing it.

Arguably the defect there lies with time.Duration rather than net/http.Server. The same class of errors is possible for any code that manipulates Duration values, so the fix should be a general fix in the time package or some (static or dynamic) analysis tool. (#20757 is closely related.)