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: the comment of Server.IdleTimeout doesn't describe its behavior correctly #32053

Closed
spacewander opened this issue May 15, 2019 · 7 comments
Labels
Milestone

Comments

@spacewander
Copy link
Contributor

@spacewander spacewander commented May 15, 2019

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

$ go version
go version go1.12.5 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
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/lzx/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/lzx/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="/usr/bin/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-build730423934=/tmp/go-build -gno-record-gcc-switches"

What did you see instead?

The comment of IdleTimeout says:

If IdleTimeout is zero, the value of ReadTimeout is used. If both are zero, ReadHeaderTimeout is used.

https://github.com/golang/go/blob/master/src/net/http/server.go#L2524

However, after reading:
https://github.com/golang/go/blob/master/src/net/http/server.go#L3008
https://github.com/golang/go/blob/master/src/net/http/server.go#L1913

It seems that IdleTimeout doesn't take account of ReadHeaderTimeout. Maybe I have missed something?

@bradfitz bradfitz added the NeedsFix label May 15, 2019
@bradfitz bradfitz added this to the Go1.13 milestone May 15, 2019
@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented May 15, 2019

Thanks. That does look like a mistake.

@jamdagni86

This comment has been minimized.

Copy link
Contributor

@jamdagni86 jamdagni86 commented May 16, 2019

@bradfitz can I send a CL for this?

@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented May 16, 2019

@jamdagni86, sure? But I haven't looked at it yet. It might be a code change or a doc fix. I don't know. So you should investigate and your commit message should explain.

@jamdagni86

This comment has been minimized.

Copy link
Contributor

@jamdagni86 jamdagni86 commented May 17, 2019

@bradfitz the current documentation is slightly incorrect. The earlier comment was

IdleTimeout is the maximum amount of time to wait for the
next request when keep-alives are enabled. If IdleTimeout
is zero, the value of ReadTimeout is used. If both are
zero, there is no timeout.

This got changed to the current version in this CL as a fix for this.

The correct sequence is this. At the end of the current request, it sets deadline for IdleTimeout or ReadTimeout, whichever is non zero. At the beginning of the next request, it sets deadline for ReadHeaderTimeout or ReadTimeout, whichever is non zero. The missing piece in the documentation is the last part where ReadTimeout is considered yet again.

In case IdleTimeout and ReadHeaderTimeout are both zeros and ReadTimeout is non zero, it sets up two timeouts with ReadTimeout duration and if none of the former two are zeros, it would end up setting two timeouts - IdleTimeout & ReadHeaderTimeout.

I'm not sure if this was the intended behaviour. And assuming that the code is working as intended, I guess it is enough if we revert the IdleTimeout documentation to the previous version and add a line in the ReadHeaderTimeout to mention that it falls back to ReadTimeout in case the former is zero.

Please let me know if this sounds good; I will send a CL.

jamdagni86 added a commit to jamdagni86/go that referenced this issue May 21, 2019
> IdleTimeout is the maximum amount of time to wait for the
> next request when keep-alives are enabled. If IdleTimeout
> is zero, the value of ReadTimeout is used. If both are
> zero, there is no timeout.

This got changed to the current version in this CL - https://go-review.googlesource.com/46434 as a fix for golang#20383.

The correct sequence is this. At the end of the current request, it sets a deadline for IdleTimeout or ReadTimeout, whichever is non zero. At the beginning of the next request, it sets a deadline for ReadHeaderTimeout or ReadTimeout, whichever is non zero. The missing piece in the documentation is the latter.

Fixes golang#32053
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented May 21, 2019

Change https://golang.org/cl/178337 mentions this issue: net/http: the comment of Server.IdleTimeout doesn't describe its behavior correctly

@spacewander

This comment has been minimized.

Copy link
Contributor Author

@spacewander spacewander commented May 27, 2019

Maybe we could simply merge the patch and close the issue?

@odeke-em

This comment has been minimized.

Copy link
Member

@odeke-em odeke-em commented Jun 16, 2019

Thank you for filing this issue @spacewander and for working on the CL @jamdagni86, plus thanks for the tag @agnivade!

So from my reading of the current situation, I agree with @spacewander's comments:

  1. s.idleTimeout() firstly checks if s.IdleTimeout is set, lest uses s.ReadTimeout as per

    go/src/net/http/server.go

    Lines 3008 to 3013 in 06e34e5

    func (s *Server) idleTimeout() time.Duration {
    if s.IdleTimeout != 0 {
    return s.IdleTimeout
    }
    return s.ReadTimeout
    }

and if s.idleTimeout()'s result (either s.IdleTimeout or s.ReadTimeout) is non-zero, we'll set the ReadDeadline on the underlying s.c.rwc and then peek(4), thus wait a max deadline as per

go/src/net/http/server.go

Lines 1913 to 1919 in 06e34e5

if d := c.server.idleTimeout(); d != 0 {
c.rwc.SetReadDeadline(time.Now().Add(d))
if _, err := c.bufr.Peek(4); err != nil {
return
}
}
c.rwc.SetReadDeadline(time.Time{})

and at the end remove that deadline. If either of s.IdleTimeout and s.ReadTimeout are not set, there won't be a deadline as we see on line 1919

c.rwc.SetReadDeadline(time.Time{})

  1. ReadHeaderTimeout+ReadTimeout in s.readRequest are disjoint from s.idleTimeout() as per point 1
    w, err := c.readRequest(ctx)

    go/src/net/http/server.go

    Lines 936 to 952 in 06e34e5

    func (c *conn) readRequest(ctx context.Context) (w *response, err error) {
    if c.hijacked() {
    return nil, ErrHijacked
    }
    var (
    wholeReqDeadline time.Time // or zero if none
    hdrDeadline time.Time // or zero if none
    )
    t0 := time.Now()
    if d := c.server.readHeaderTimeout(); d != 0 {
    hdrDeadline = t0.Add(d)
    }
    if d := c.server.ReadTimeout; d != 0 {
    wholeReqDeadline = t0.Add(d)
    }
    c.rwc.SetReadDeadline(hdrDeadline)

Thus this warrants just a documentation change the doc for Server.IdleTimeout from

go/src/net/http/server.go

Lines 2525 to 2527 in 06e34e5

// next request when keep-alives are enabled. If IdleTimeout
// is zero, the value of ReadTimeout is used. If both are
// zero, ReadHeaderTimeout is used.

to

 // next request when keep-alives are enabled. If IdleTimeout 
 // is zero, the value of ReadTimeout is used. If both are 
 // zero, there is no timeout.

as well as update the doc for ReadHeaderTimeout to include falling back to ReadTimeout as @jamdagni86 has presented in their CL.

jamdagni86 added a commit to jamdagni86/go that referenced this issue Jun 18, 2019
> IdleTimeout is the maximum amount of time to wait for the
> next request when keep-alives are enabled. If IdleTimeout
> is zero, the value of ReadTimeout is used. If both are
> zero, there is no timeout.

This got changed to the current version in this CL - https://go-review.googlesource.com/46434 as a fix for golang#20383.

The correct sequence is this. At the end of the current request, it sets a deadline for IdleTimeout or ReadTimeout, whichever is non zero. At the beginning of the next request, it sets a deadline for ReadHeaderTimeout or ReadTimeout, whichever is non zero. The missing piece in the documentation is the latter.

Fixes golang#32053
@gopherbot gopherbot closed this in 1962dc8 Jun 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.