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: add InactivityTimeout to http.DefaultClient #22982

Open
knightXun opened this Issue Dec 4, 2017 · 8 comments

Comments

Projects
None yet
6 participants
@knightXun

knightXun commented Dec 4, 2017

var DefaultClient = &Client{}

Look at two code:
server.go:

package main

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


func main() {
	http.HandleFunc("/test", test)
	http.ListenAndServe(":9999", nil)
}

func test(w http.ResponseWriter, r *http.Request){
	fmt.Println("test func")
	time.Sleep(100000000000000)
}

client.go

package main

import (
	"net/http"
	"fmt"
)

func main() {
	resp, err := http.Get("http://127.0.0.1:9999/test")
	if err != nil {
		fmt.Println(err.Error())
	} else {
		fmt.Println(resp.StatusCode)
	}
}

when we run client.go, it will stuck. please notice it.

@mvdan

This comment has been minimized.

Member

mvdan commented Dec 4, 2017

What change in particular are you suggesting? That a timeout of 0 convert to some default, or that the DefaultClient declaration add one?

The former will break many programs, as there would be no way to disable the timeout. The latter would still break some, as they may depend on DefaultClient not having a timeout.

And there's always the question of what timeout to use. If it's too short, it will break far too many programs. If it's too long, to some people it will still "get stuck".

Why not use a custom http.Client? In your client program, it would be one extra line and you'd be able to use whatever timeout suits you.

@bradfitz bradfitz changed the title from Default httpClient shoud add timeout param. to net/http: add Timeout to http.DefaultClient? Dec 4, 2017

@bradfitz bradfitz added this to the Go1.11 milestone Dec 4, 2017

@bradfitz

This comment has been minimized.

Member

bradfitz commented Dec 4, 2017

Maybe we can do a 3 minute timeout or something high.

Enough other stuff breaks or times out around 3 minutes in the wild that this doesn't seem too invasive.

Thoughts, @tombergan?

@CAFxX

This comment has been minimized.

Contributor

CAFxX commented Dec 4, 2017

I don't want to hijack this specific issue but I fully agree with all network operations (i.e. not just HTTP) having timeouts by default. We have been bitten so many times by components that don't allow to specify timeouts for network operations (or that have no sane default timeout) that is now one of the first things I check when evaluating the use of a new component/library.

@knightXun

This comment has been minimized.

knightXun commented Dec 12, 2017

Maybe 3 minute is enough.

@gopherbot

This comment has been minimized.

gopherbot commented Jun 5, 2018

Change https://golang.org/cl/116356 mentions this issue: net/http: add 5 minute Timeout to DefaultClient

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jun 5, 2018

I sent CL 116356 to add a 5 minute timeout, but right after I mailed it I realized it would break people wanting to do long downloads.

I think the only conservative thing we could do by default is to add some sort of InactivityTimeout, but it's too late for that, so bumping to Go 1.12.

@bradfitz bradfitz modified the milestones: Go1.11, Go1.12 Jun 5, 2018

@knightXun

This comment has been minimized.

knightXun commented Jun 6, 2018

thanks @bradfitz

@rsc rsc added the NeedsFix label Jun 11, 2018

@rsc rsc changed the title from net/http: add Timeout to http.DefaultClient? to net/http: add InactivityTimeout to http.DefaultClient Jun 11, 2018

@rsc

This comment has been minimized.

Contributor

rsc commented Jun 11, 2018

Per discussion with proposal review, retitled with understanding that step 2 is to set it to a good default in #24138.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment