webapp-improved icon indicating copy to clipboard operation
webapp-improved copied to clipboard

Remove uneccesary request_body_tempfile_limit override from webob

Open kofalt opened this issue 9 years ago • 0 comments

The request_body_tempfile_limit key is only used in a single place in webapp2:

# grep -r request_body_tempfile_limit
webob/request.py:    request_body_tempfile_limit = 10*1024
webob/request.py:        ``self.request_body_tempfile_limit``
webob/request.py:        tempfile_limit = self.request_body_tempfile_limit
webapp2.py:    request_body_tempfile_limit = 0

Specifically, in webob's _copy_body_tempfile, which cancels the use of a tempfile if the content is too small:

if not tempfile_limit or todo <= tempfile_limit:
    return False

Setting this key to zero has the unintended effect of never using a tempfile, which will cause webob's copy_body function to always read a body straight into memory.

did_copy = self._copy_body_tempfile()
if not did_copy:
    # it wasn't necessary, so just read it into memory
    self.body = self.body_file.read(self.content_length)

This is clearly wrong, as the body is of arbitrary size and will crash if the request is larger than your available memory. Presumably, the intended effect was to have a body of any size be written to a tempfile, but webob already has a sensible default:

## The limit after which request bodies should be stored on disk
## if they are read in (under this, and the request body is stored
## in memory):
request_body_tempfile_limit = 10*1024

When chasing this back, it looks like this override was simply a mistake during import:

# hg blame webapp2.py  | grep -C 5 request_body_tempfile_limit
...
255:     # Attributes from webapp.
255:     request_body_tempfile_limit = 0
...

# hg log | grep -C 5 '255:'
...
changeset:   255:a24a0f6231af
user:        Rodrigo Moraes <[email protected]>
date:        Thu Jun 16 16:00:29 2011 -0300
summary:     Copied Request from webapp, so that the whole API is available without SDK.
...

In summary, it is my belief that overriding this value causes the opposite of its intended effect and further is not necessary.

kofalt avatar Jul 08 '15 23:07 kofalt