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

Response timeout is harmful - remove it #1625

Closed
jaraco opened this Issue Aug 18, 2017 · 6 comments

Comments

Projects
None yet
3 participants
@jaraco
Member

jaraco commented Aug 18, 2017

  • I'm submitting a ...
    [X ] bug report
    [ ] feature request
    [ ] question about the decisions made in the repository

  • What is the current behavior?

As described in this answer, when CherryPy raises a TimeoutError, it's doing so because the request took longer than the timeout to complete, but it was still allowed to complete.

The docs say:

response.timeout: the number of seconds to allow responses to run

  • If the current behavior is a bug, please provide the steps to reproduce and if possible a screenshots and logs of the problem.

Consider this script:

__requires__ = ['cherrypy']

import time
import os

import cherrypy


class Server:
	@cherrypy.expose
	def index(self):
		time.sleep(5)
		return 'done!'

	@classmethod
	def run(cls):
		config = {
			'global': {
				'server.socket_port': int(os.environ.get('PORT', 8080)),
				'engine.timeout_monitor.frequency': 1,
				'response.timeout': 2,
			},
		}
		cherrypy.quickstart(cls(), config=config)


__name__ == '__main__' and Server.run()

Run it with rwt:

$ PORT=8081 rwt -- test-timeout.py
Collecting cherrypy
  Using cached CherryPy-11.0.0-py2.py3-none-any.whl
Collecting six (from cherrypy)
  Using cached six-1.10.0-py2.py3-none-any.whl
Collecting cheroot>=5.2.0 (from cherrypy)
  Using cached cheroot-5.8.3-py2.py3-none-any.whl
Collecting portend>=2.1.1 (from cherrypy)
  Using cached portend-2.1.2-py2.py3-none-any.whl
Collecting tempora>=1.8 (from portend>=2.1.1->cherrypy)
  Using cached tempora-1.9-py2.py3-none-any.whl
Collecting pytz (from tempora>=1.8->portend>=2.1.1->cherrypy)
  Using cached pytz-2017.2-py2.py3-none-any.whl
Installing collected packages: six, cheroot, pytz, tempora, portend, cherrypy
Successfully installed cheroot-5.8.3 cherrypy-11.0.0 portend-2.1.2 pytz-2017.2 six-1.10.0 tempora-1.9
[18/Aug/2017:13:22:38] ENGINE Listening for SIGTERM.
[18/Aug/2017:13:22:38] ENGINE Listening for SIGHUP.
[18/Aug/2017:13:22:38] ENGINE Listening for SIGUSR1.
[18/Aug/2017:13:22:38] ENGINE Bus STARTING
[18/Aug/2017:13:22:38] ENGINE Started monitor thread 'Autoreloader'.
[18/Aug/2017:13:22:38] ENGINE Started monitor thread '_TimeoutMonitor'.
[18/Aug/2017:13:22:38] ENGINE Serving on http://127.0.0.1:8081
[18/Aug/2017:13:22:38] ENGINE Bus STARTED

Then time a request with curl:

$ time curl http://localhost:8081/
Unrecoverable error in the server.
Traceback (most recent call last):
  File "/var/folders/c6/v7hnmq453xb6p2dbz1gqc6rr0000gn/T/rwt-abvklbkv/cherrypy/_cpwsgi.py", line 190, in trap
    return func(*args, **kwargs)
  File "/var/folders/c6/v7hnmq453xb6p2dbz1gqc6rr0000gn/T/rwt-abvklbkv/cherrypy/_cpwsgi.py", line 106, in __call__
    return self.nextapp(environ, start_response)
  File "/var/folders/c6/v7hnmq453xb6p2dbz1gqc6rr0000gn/T/rwt-abvklbkv/cherrypy/_cpwsgi.py", line 439, in tail
    return self.response_class(environ, start_response, self.cpapp)
  File "/var/folders/c6/v7hnmq453xb6p2dbz1gqc6rr0000gn/T/rwt-abvklbkv/cherrypy/_cpwsgi.py", line 245, in __init__
    self.run()
  File "/var/folders/c6/v7hnmq453xb6p2dbz1gqc6rr0000gn/T/rwt-abvklbkv/cherrypy/_cpwsgi.py", line 348, in run
    request.run(meth, path, qs, rproto, headers, rfile)
  File "/var/folders/c6/v7hnmq453xb6p2dbz1gqc6rr0000gn/T/rwt-abvklbkv/cherrypy/_cprequest.py", line 617, in run
    raise cherrypy.TimeoutError()
cherrypy._cperror.TimeoutError
curl http://localhost:8081/  0.01s user 0.01s system 0% cpu 5.028 total

The request takes the full 5 seconds to complete even though the timeout was set to 2 seconds. Additionally, the test will complete without timing out (in most cases) if the timeout_monitor frequency isn't increased.

  • What is the expected behavior?

If we were to believe the docs, the request would not be allowed to run longer than the timeout period, but it clearly is allowed to run to completion. Yet, even though it's allowed to run to completion, an error is returned, losing the value of the computation even if the client was still connected to receive it. According to the referenced SO answer, it's difficult if not impossible to interrupt a thread safely. Since the request has to be allowed to run to completion, the "timeout" functionality is not behaving as a timeout but is instead behaving as "make fail if slow". Such behavior should be optional and disabled by default if it's supplied at all.

  • What is the motivation / use case for changing the behavior?

The current implementation is misleading and causes harm.

Consider the condition where a user has just invoked a long-running request. They've left it to run to see if it will complete and are eager to see what the result is. The request runs for 10 minutes and finally completes, but when it does, the result is discarded and replaced with a timeout traceback.

Better would be for CherryPy docs to indicate that it cannot safely time out long-running requests and that requests that take too long should be managed out of process.

  • Please tell us about your environment:
  • Python version: 3.6.2
  • OS: macOS 10.12.2
  • Browser: curl

@jaraco jaraco changed the title from Request timeout is worthless - remove it to Response timeout is worthless - remove it Aug 18, 2017

@jaraco

This comment has been minimized.

Show comment
Hide comment
@jaraco

jaraco Aug 18, 2017

Member

Hmm. I now see the docs also say:

You are free to [periodically check for timeout] within your own code.

Perhaps it could say "you must" instead of "you are free". But still, I'm struggling to think of a use-case where this timeout functionality is generally useful. Perhaps it would make sense for CherryPy to provide the timeout functionality, but it seems like it shouldn't throw away a response after it's been completed, even it exceeded the timeout to produce it.

I would like to solicit feedback to find out if this feature is used and useful by others.

Member

jaraco commented Aug 18, 2017

Hmm. I now see the docs also say:

You are free to [periodically check for timeout] within your own code.

Perhaps it could say "you must" instead of "you are free". But still, I'm struggling to think of a use-case where this timeout functionality is generally useful. Perhaps it would make sense for CherryPy to provide the timeout functionality, but it seems like it shouldn't throw away a response after it's been completed, even it exceeded the timeout to produce it.

I would like to solicit feedback to find out if this feature is used and useful by others.

@jaraco jaraco changed the title from Response timeout is worthless - remove it to Response timeout is harmful - remove it Aug 18, 2017

@webknjaz

This comment has been minimized.

Show comment
Hide comment
@webknjaz
Member

webknjaz commented Aug 21, 2017

@Safihre

This comment has been minimized.

Show comment
Hide comment
@Safihre

Safihre Aug 31, 2017

Contributor

Agreed to all of it!

Contributor

Safihre commented Aug 31, 2017

Agreed to all of it!

@webknjaz

This comment has been minimized.

Show comment
Hide comment
@webknjaz

webknjaz Sep 2, 2017

Member

Yes, docs need to be more clear and probably include more references. Also this feature needs to be better advertised on the Internet.

Member

webknjaz commented Sep 2, 2017

Yes, docs need to be more clear and probably include more references. Also this feature needs to be better advertised on the Internet.

Safihre added a commit to sabnzbd/sabnzbd that referenced this issue Sep 2, 2017

Disable CherryPy timeout monitor
Besides throwing errors, it doesn't really help anything. The actions still get performed. 
See also: cherrypy/cherrypy#1625

@jaraco jaraco closed this in c4e1064 Nov 18, 2017

@webknjaz

This comment has been minimized.

Show comment
Hide comment
@webknjaz

webknjaz Jul 16, 2018

Member

FTR, I've found a few ways to actually make it work correctly:

@jaraco would we want to get back this feature if it'll be made consistent?

Member

webknjaz commented Jul 16, 2018

FTR, I've found a few ways to actually make it work correctly:

@jaraco would we want to get back this feature if it'll be made consistent?

@jaraco

This comment has been minimized.

Show comment
Hide comment
@jaraco

jaraco Jul 16, 2018

Member

Yes, I think it would be nice to have a timeout for generating responses, now that it seems it may be possible. It would be nice if such a feature could be implemented as a tool in a separate package (though I'm not sure if that's possible). I'd also like to see any timeout support be explicit about what the timeout or timeouts mean. Especially in a world with large and streaming responses, there's a big difference between a request that times out before the response is started and a request that times out mid-response (with the former being more useful in practice).

Member

jaraco commented Jul 16, 2018

Yes, I think it would be nice to have a timeout for generating responses, now that it seems it may be possible. It would be nice if such a feature could be implemented as a tool in a separate package (though I'm not sure if that's possible). I'd also like to see any timeout support be explicit about what the timeout or timeouts mean. Especially in a world with large and streaming responses, there's a big difference between a request that times out before the response is started and a request that times out mid-response (with the former being more useful in practice).

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