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

Open
rsc opened this issue Feb 26, 2018 · 12 comments
Open

net/http: make default configs have better timeouts #24138

rsc opened this issue Feb 26, 2018 · 12 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

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

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

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

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

/cc @bradfitz

@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 13, 2018
@FiloSottile
Copy link
Contributor

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

@bradfitz
Copy link
Contributor

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

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

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

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

szuecs commented Dec 3, 2018

@FiloSottile a separate issue is already available #23427

@gofish
Copy link

gofish commented Aug 1, 2023

Would setting a default ReadHeaderTimeout for http.[ListenAnd]Serve[TLS] break streaming use cases?

This is now flagged as gosec rule G114, which applies to all documented uses of these functions:

The recommendation is to at least set a ReadHeaderTimeout to mitigate slowloris for simple exposed HTTP handlers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

8 participants