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: make default configs have better timeouts #24138

Open
rsc opened this Issue Feb 26, 2018 · 11 comments

Comments

Projects
None yet
7 participants
@rsc
Contributor

rsc commented Feb 26, 2018

See #23459.

Client, Server, and Transport may all have timeout fields in which zero = infinity. Instead it should be a reasonable default.

@rsc rsc added this to the Go1.11 milestone Feb 26, 2018

@FiloSottile

This comment has been minimized.

Member

FiloSottile commented Feb 26, 2018

This issue is close to my heart (1, 2, #16100) but while I agree there should be reasonable defaults, I think changing them now would be a full violation of Go 1 compatibility.

A default Server or Client can serve streaming requests today, with any timeout they couldn't.

@mvdan

This comment has been minimized.

Member

mvdan commented Feb 27, 2018

Perhaps 1.11 should have high, non-infinity default timeouts, letting people adapt with less of a shock before setting lower, more sensible defaults in 1.12.

@FiloSottile

This comment has been minimized.

Member

FiloSottile commented Feb 27, 2018

I don't think the way we transition matters, IMHO it's a Go 1 compatibility promise violation: there's a specific real documented use case (streaming) that would stop working from one version to the other.

If we want to consider this a security exception, I'm not sure where I stand, but I think that's the frame in which we should evaluate it.

@artyom

This comment has been minimized.

Contributor

artyom commented Feb 27, 2018

Perhaps the less intrusive change wrt backwards compatibility would be to only set non-zero timeouts on package-level http.ListenAndServe{,TLS} calls which create implicit Server instance, but keep them unchanged on explicitly created Servers?

@andybons

This comment has been minimized.

Member

andybons commented Mar 13, 2018

/cc @bradfitz

@FiloSottile

This comment has been minimized.

Member

FiloSottile commented Mar 30, 2018

At least we should mention the risk in the docs. See #22085

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jun 7, 2018

In #22982, an instance of this meta bug, I said in #22982 (comment) :

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.

@FiloSottile

This comment has been minimized.

Member

FiloSottile commented Sep 5, 2018

Recording here an idea that someone at GopherCon suggested. (I don't remember who, apologies.)

Even if we can't change the DefaultClient behavior, we could add a DefaultClientWithTimeout, which although ugly and a mouthful, would prominently draw attention to the importance of client timeouts ("officially" endorsing it as a default, and giving us a place in the docs to elaborate), and provide a way to use them that's easy, maintained, and efficient (globally reusing connections).

Not sure how to map this to the server side, though.

@szuecs

This comment has been minimized.

szuecs commented Nov 29, 2018

Honestly, I think the DefaultClient is pretty much broken. If you try to traffic switch by changing DNS you can wait forever (or does it recently changed?).
@FiloSottile don’t you think my example is more a bug?

I am also fine with DefaultClientWithTimeout, but something should be changed in go1.

@FiloSottile

This comment has been minimized.

Member

FiloSottile commented Dec 1, 2018

@szuecs If I understand correctly, the issue is that DefaultClient does not respond to changes in the DNS? I can see that being intended behavior due to connection reuse, or maybe due to hostname lookup caching, but in any case that's unrelated to request/response timeouts. Can you open a separate issue and expand on what you expected to happen vs. what actually happened? Thank you!

@szuecs

This comment has been minimized.

szuecs commented Dec 3, 2018

@FiloSottile a separate issue is already available #23427

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