Opened 9 years ago

Closed 12 months ago

#3746 defect closed fixed (fixed)

twisted.web timeout of 12 hours is too long

Reported by: ivank Owned by: Glyph
Priority: high Milestone:
Component: web Keywords:
Cc: Thijs Triemstra, Jean-Paul Calderone Branch: 3746-lukasa-tweb-timeout
branch-diff, diff-cov, branch-cov, buildbot
Author:

Description (last modified by Thijs Triemstra)

Firefox tries to keep the connection open for 5 minutes; IE 1 or 2 minutes. Right now, Twisted keeps connections open for 12 hours if the browser does not close them. Apache's default timeout is 15 seconds, and nginx's is 65 seconds. A short timeout would not disrupt responses that are just taking a while to generate.

The timeout is in three places:

  • twisted/web/http.py: timeOut = 60 * 60 * 12
  • twisted/web/http.py: def __init__(self, logPath=None, timeout=60*60*12):
  • twisted/web/server.py: def __init__(self, resource, logPath=None, timeout=60*60*12):

Attachments (1)

reduce_http_timeout_3746.patch (859 bytes) - added by Rotund 8 years ago.
Reduce default timeout to 15 seconds

Download all attachments as: .zip

Change History (19)

comment:1 Changed 9 years ago by Thijs Triemstra

Cc: Thijs Triemstra added
Description: modified (diff)

add markup to ticket description

comment:2 Changed 9 years ago by Jean-Paul Calderone

Cc: Jean-Paul Calderone added

The timeout is a public feature of the API. Applications can set it to anything they like. I would be interested in adding some disconnect pressure based on how close the process is to its file descriptor limit, but I don't think I'm very interested in playing guess-the-timeout-that-is-appropriate-for-all-applications.

#816 and #1502 are somewhat related to this, I think.

Unless you have a motivation for timing out connections for a different reason?

comment:3 in reply to:  2 Changed 9 years ago by Thijs Triemstra

Replying to exarkun:

I would be interested in adding some disconnect pressure based on how close the process is to its file descriptor limit, but I don't think I'm very interested in playing guess-the-timeout-that-is-appropriate-for-all-applications.

That 12 hours is already a guess-the-timeout-that-is-appropriate-for-all-applications, but simply too much :) Why not lower the default to something that's closer to Apache's value? It won't change the fact it's a public feature of the API and applications will still be able to set it. It's just hard to imagine that any Twisted application would use a 12 hour timeout vs the values other webservers use..

comment:4 Changed 9 years ago by ivank

My description was incorrect. At least in Firefox, 'network.http.keep-alive.timeout' = 300 is just a hint sent to the webserver (as Keep-Alive: 300). Firefox keeps the connection open until the webserver closes it.

So, by default, a Firefox visitor loading page on unproxied twisted.web website would keep the connection open for 12 hours. I don't think anyone should expect this to happen.

comment:5 Changed 9 years ago by jknight

Agreed, 12 hours is way too long. I made web2 have two timeouts: one is 15sec (between requests) and one is 4 minutes (while reading a request). There is no timeout once the request has finished being read until the server finishes writing the response. Those are good default values: twisted.web should copy them.

comment:6 Changed 9 years ago by ivank

Keywords: security added
Priority: normalhigh

This has security consequences because the default timeout value makes it very easy to intentionally or accidentally DoS a twisted.web server.

comment:7 Changed 9 years ago by ivank

Lowering the timeout requires implementing a 'noisy' mode for HTTPChannel, or else the log will be filled with messages from:

log.msg("Timing out client: %s" % str(self.transport.getPeer()))

The noisy mode might also log connecting and disconnecting clients.

Do 'noisy' messages require tests?

comment:8 Changed 8 years ago by Jean-Paul Calderone

I'm not sure that log message is worth keeping.

comment:9 Changed 8 years ago by Rotund

Cherokee also defaults to 15 seconds.

Changed 8 years ago by Rotund

Reduce default timeout to 15 seconds

comment:10 Changed 7 years ago by <automation>

Owner: jknight deleted

comment:11 Changed 7 years ago by ivank

Another thing we could do in this branch is to convince browsers to close the connection by sending them a Keep-Alive: timeout=N header[1]. This would reduce the number of TIME_WAIT sockets on the server.

[1] browser behavior varies; see http://wiki.nginx.org/HttpCoreModule#keepalive_timeout

comment:12 Changed 17 months ago by hawkowl

Branch: 3746-tweb-timeout

comment:13 Changed 17 months ago by hawkowl

Keywords: review added; security removed

I updated this; please review.

comment:14 Changed 17 months ago by Adi Roiban

Keywords: review removed
Owner: set to hawkowl

Changes look good. 5 minutes is very conservative for my taste :)

we can argue forever on this default value... but I have another comment: where is the associated test, I don't see any changes in the existing tests ?

Is it ok to merge without test coverage?

Please check my comments and merge, or fix and merge.

Thanks!

comment:15 Changed 12 months ago by Cory Benfield

Branch: 3746-tweb-timeout3746-lukasa-tweb-timeout
Keywords: review added
Owner: hawkowl deleted

Ok, I'm taking this back up from hawkie.

Like adi, I think that five minutes is too conservative, so I've updated hawkie's patch to do a one minute timeout instead. I've also added a test of that default behaviour. Please re-review.

comment:16 Changed 12 months ago by Cory Benfield <lukasaoz@…>

In 396071d3:

Fix the name of the topfile for #3746.

comment:17 Changed 12 months ago by Glyph

Keywords: review removed
Owner: set to Glyph

Thanks! Just waiting for the builders.

comment:18 Changed 12 months ago by Glyph <glyph@…>

Resolution: fixed
Status: newclosed

In 08795793:

Merge 3746-lukasa-tweb-timeout: Brief description

Author: hawkowl, lukasa

Reviewer: adiroiban, glyph

Fixes: #3746

twisted.web.http.HTTPFactory now times connections out after one minute of no data from the client being received, before the request is complete, rather than twelve hours.

Note: See TracTickets for help on using tickets.