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

Jump to bottom

net/http: add InactivityTimeout to http.DefaultClient #22982

Closed
knightXun opened this issue Dec 4, 2017 · 11 comments
Closed

net/http: add InactivityTimeout to http.DefaultClient #22982

knightXun opened this issue Dec 4, 2017 · 11 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@knightXun
Copy link

knightXun commented Dec 4, 2017 •
edited

go/src/net/http/client.go

Line 110 in 1b9f663

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
Copy link
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 Default httpClient shoud add timeout param. net/http: add Timeout to http.DefaultClient? Dec 4, 2017
@bradfitz bradfitz added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Dec 4, 2017
@bradfitz bradfitz added this to the Go1.11 milestone Dec 4, 2017
@bradfitz
Copy link
Contributor

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
Copy link
Contributor

CAFxX commented Dec 4, 2017 •
edited

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
Copy link
Author

Maybe 3 minute is enough.

@CAFxX CAFxX mentioned this issue Jan 17, 2018
Closed
@tristangio tristangio mentioned this issue Mar 1, 2018
Closed
@gopherbot
Copy link

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

@bradfitz
Copy link
Contributor

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
Copy link
Author

thanks @bradfitz

@bradfitz bradfitz mentioned this issue Jun 7, 2018
Open
@rsc rsc added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 11, 2018
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jun 11, 2018
@rsc rsc changed the title net/http: add Timeout to http.DefaultClient? net/http: add InactivityTimeout to http.DefaultClient Jun 11, 2018
@rsc
Copy link
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.

@knightXun knightXun mentioned this issue Aug 30, 2018
Merged
@andybons andybons modified the milestones: Go1.12, Go1.13 Feb 12, 2019
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@zhxqgithub zhxqgithub mentioned this issue Oct 18, 2019
Open
@networkimprov
Copy link

This was closed without reference to a CL. Was it fixed?

@mvdan
Copy link
Member

mvdan commented Dec 14, 2021

It doesn't look fixed, and I'm not sure why @knightXun closed the issue. It's their decision to close it, though.

@networkimprov
Copy link

cc @neild

@golang golang locked and limited conversation to collaborators Dec 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
8 participants
@bradfitz @rsc @andybons @CAFxX @networkimprov @mvdan @knightXun @gopherbot