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: closeNotify does not work well when read timeout is set #9524

Closed
xiang90 opened this issue Jan 6, 2015 · 11 comments
Closed

net/http: closeNotify does not work well when read timeout is set #9524

xiang90 opened this issue Jan 6, 2015 · 11 comments
Milestone

Comments

@xiang90
Copy link

xiang90 commented Jan 6, 2015

There is a read timeout configuration for http server:

ReadTimeout    time.Duration // maximum duration before timing out read of the request

I expect it is the timeout for finishing reading the request only.

When read timeout is set, the connection will be considered as closed after the timeout even if the connection is still open and working. The following code set up a http server with read timeout and a handler simply blocks forever until closeNotify fires.

I expect to see the hander blocks "forever", but it unblocks after the read timeout.

The closeNotifiy depends on the read of the underlying connection. Shall we cancel the read timeout after finishing reading the request?

package main

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

func main() {
    srv := &http.Server{
        Addr:        "localhost:8080",
        Handler:     myhandler{},
        ReadTimeout: time.Second,
    }
    go func() {
        // wait server to start
        time.Sleep(time.Second)
        resp, err := http.Get("http://" + srv.Addr)
        if err != nil {
            log.Println(err)
            return
        }
        resp.Body.Close()
    }()
    log.Fatal(srv.ListenAndServe())
}

type myhandler struct{}

func (h myhandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
    if cn, ok := w.(http.CloseNotifier); ok {
        <-cn.CloseNotify()
        log.Panicf("unexpected closed client")
    }
}
@rsc
Copy link
Contributor

rsc commented Jun 29, 2015

Too late for Go 1.5.

@rsc rsc modified the milestones: Unplanned, Go1.5 Jun 29, 2015
@bwplotka
Copy link

Any update on that? We have Go 1.7 and this is still not fixed. We just lost one day investigating the bug which was caused by the ReadTimeout behaving totally different than stated in documentation:

ReadTimeout  time.Duration // maximum duration before timing out read of the request

The above is not true while using CloseNotify channel. As stated in @xiang90 description, for CloseNotify this is an absolute max duration before timing out of the whole connection.

Surprisingly, the WriteTimeout works as expected and it is using the same overwriting the Deadline mechanism.

Any plans for this bug?

@bradfitz
Copy link
Contributor

A lot of work happened in this space in Go 1.8. Notably,
c7e0dda and faf882d.

I lost this bug because it fell into the "Unplanned" milestone, which we no longer use. I'll retarget this for Go 1.9.

But it might also be fixed by the Go 1.8 changes. Can you investigate using Go 1.8beta2?

See https://groups.google.com/forum/#!topic/golang-nuts/5YvvkQjJJW0 for details.

$ go get golang.org/x/build/version/go1.8beta2

@bradfitz bradfitz modified the milestones: Go1.9, Unplanned Dec 16, 2016
@bwplotka
Copy link

Thanks, will try in the next days.

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/34515 mentions this issue.

@bwplotka
Copy link

bwplotka commented Dec 18, 2016

@bradfitz: I can confirm that I can reproduce this issue on go version go1.8beta2 linux/amd64 and on the current master.

I dived even deeper into the code and used this occasion to start my contribution to Go ! (:

Sorry that I did not discuss the change before doing the CL, but maybe it will be easier to iterate over the initial attempt: https://go-review.googlesource.com/#/c/34515/

The main problem is that even the updated description is not entirely true:

// ReadTimeout is the maximum duration for reading the entire
// request, including the body.

// Because ReadTimeout does not let Handlers make per-request
// decisions on each request body's acceptable deadline or
// upload rate, most users will prefer to use
// ReadHeaderTimeout. It is valid to use them both.

Reading the whole body does not reset the ReadTimeout Deadline. ReadTimeout Deadline is being reset (or replaced by IdleTimeout) in the very end of the conn serve method for every request. As a result timeout can exceed while being in handler. In my opinion we should update the description that this Timeout also includes time spend in the Handler (which I did in the CL with the test case for that). My only worry is that now ReadTimeout is not really for... Read but rather RequestTimeout. (:

Any ideas or feedback? Naturally, I am new to the Go contribution and net/http internal code, so I could make a mistake in my thinking in these area.

@bradfitz
Copy link
Contributor

I think this was fixed between Go 1.8beta2 and Go 1.8rc1 in https://go-review.googlesource.com/34813 while fixing #18447.

@Bplotka, @xiang90, could either of you two confirm?

@bwplotka
Copy link

bwplotka commented Feb 1, 2017

Yes, it seems that the issue is fixed in Go 1.8rc1

@odeke-em
Copy link
Member

odeke-em commented Feb 5, 2017

@bradfitz, @Bplotka confirmed it.
Running the test program:

@xiang90
Copy link
Author

xiang90 commented Feb 5, 2017

The original issue is fixed on tip.

@xiang90
Copy link
Author

xiang90 commented Feb 5, 2017

Shall I close this issue? @bradfitz

@bradfitz bradfitz closed this as completed Feb 5, 2017
gyuho added a commit to gyuho/etcd that referenced this issue Apr 13, 2017
ref. golang/go#9524 (comment)

Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
heyitsanthony pushed a commit to heyitsanthony/etcd that referenced this issue Apr 17, 2017
ref. golang/go#9524 (comment)

Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
@golang golang locked and limited conversation to collaborators Feb 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants