Opened 13 years ago

Last modified 5 years ago

#601 defect new

ThrottlingFactory doesn't throttle static web resource

Reported by: mg Owned by: JohnDoeee
Priority: high Milestone:
Component: web Keywords:
Cc: acapnotic, itamarst, jknight, mg Branch:
Author:

Description


Attachments (1)

ticket-601-ratelimit-non-streaming-producer.patch (1.6 KB) - added by JohnDoeee 5 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 13 years ago by mg

A throttling factory doesn't seem to limit rates using the following tac:

----- web.tac -----
import sys

from twisted.application import internet, service
from twisted.web import server, static
from twisted.protocols import policies

application = service.Application('web')

site = server.Site(static.File('.'))
site = policies.ThrottlingFactory(site, sys.maxint, 1, 1)

webServer = internet.TCPServer(8080, site)
webServer.setServiceParent(application)
-------------------

I got the following error on 1st request although, afaict, the site still
responded ok.

---- exception -----
  File
"/home/matt/python-ext//lib/python2.3/site-packages/twisted/scripts/twistd.py",
line 176, in runApp
    app.runReactorWithLogging(config, oldstdout, oldstderr)
  File
"/home/matt/python-ext//lib/python2.3/site-packages/twisted/application/app.py",
line 90, in runReactorWithLogging
    reactor.run()
  File
"/home/matt/python-ext//lib/python2.3/site-packages/twisted/internet/default.py",
line 126, in run
    self.mainLoop()
  File
"/home/matt/python-ext//lib/python2.3/site-packages/twisted/internet/default.py",
line 134, in mainLoop
    self.runUntilCurrent()
--- <exception caught here> ---
  File
"/home/matt/python-ext//lib/python2.3/site-packages/twisted/internet/base.py",
line 423, in runUntilCurrent
    call.func(*call.args, **call.kw)
  File
"/home/matt/python-ext//lib/python2.3/site-packages/twisted/protocols/policies.py",
line 213, in checkWriteBandwidth
    self.throttleWrites()
  File
"/home/matt/python-ext//lib/python2.3/site-packages/twisted/protocols/policies.py",
line 238, in throttleWrites
    p.throttleWrites()
  File
"/home/matt/python-ext//lib/python2.3/site-packages/twisted/protocols/policies.py",
line 164, in throttleWrites
    self.producer.pauseProducing()
exceptions.AttributeError: 'NoneType' object has no attribute 'pauseProducing'
--------------------

comment:2 Changed 12 years ago by acapnotic

Investigating.
The addition of __getattr__ on ProtocolWrapper <a
href="http://svn.twistedmatrix.com/cvs/trunk/twisted/protocols/policies.py?r1=9765&r2=9767&p1=trunk/twisted/protocols/policies.py&p2=trunk/twisted/protocols/policies.py">in
rev 9767</a>  is the cause for the traceback -- ThrottlingProtocol and its
transport end up having a namespace collision around "producer."

haven't decided if this is also the reason for the failure to limit.

Unfortunately the ThrottlingTestCase tests are marked as "skip."

comment:3 Changed 12 years ago by acapnotic

<keturn> brilliant!  ThrottleStuff doesn't work on web.static because
web.static..pauseProducing is defined as "pass"
<foom> keturn: as it should be.
<foom> keturn: static.File is a non-streaming-producer, which means
resumeProducing should be called once every time data is wanted

so policies.ThrottlingProtocol doesn't support non-streaming producers.  It
ignores the "streaming" argument to registerProducer and doesn't change its
behaviour accordingly.

so either fix, or have it raise exceptions when it gets non-streaming producers
registered to it.

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

This might be related to #616.

comment:5 Changed 6 years ago by <automation>

Owner: acapnotic deleted

Changed 5 years ago by JohnDoeee

comment:6 Changed 5 years ago by JohnDoeee

Keywords: review added

I have added a patch that'll fix the problem, but as I am uncertain this is a good way to solve it, I would like to request a preliminary review of my method of solving the problem.

A big problem with the ThrottlingFactory is that any producer can run rampant for up to a second before it is stopped. IF the producer is done and the ThrottlingFactory attempts to pause, it runs into a problem where self.producer is None (I am not sure why that happens).

As noted elsewhere, the way the ThrottlingFactory limits the traffic is bad, but with this patch only the way to do the actual throttling needs a makeover.

What I want to know is if this is an acceptable way to do the actual limiting.

comment:7 Changed 5 years ago by Itamar Turner-Trauring

Keywords: review removed
Owner: set to JohnDoeee
  1. I am skeptical that this needs to be solved by changing twisted.internet.abstract. If the problem is that "policies.ThrottlingProtocol doesn't support non-streaming producers", the solution would be to add that support. twisted.protocols.tls._PullToPush and related code is a way to do that.
  2. In the unlikely case that it does, lack of parenthesis means I have to remember precedence rules for "and" and "or".
  3. No tests.

comment:8 Changed 5 years ago by Itamar Turner-Trauring

Hm. That review seems rather snippy. My apologies, I guess I was having a bad idea. I appreciate someone trying to fix this! Like I said, though, using something like the _PullToPush class is probably the way to go.

Note: See TracTickets for help on using tickets.