Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Memory leak when process big file upload #740

Closed
JerryKwan opened this Issue · 26 comments

5 participants

@JerryKwan

something is wrong in tornado v3.0.1
when i upload a large file (about 101M, larger than default max_buffer_size), then tornado server raise an exception as follows:
ERROR:tornado.application:Error in connection callback
Traceback (most recent call last):
File "/usr/local/lib/python2.7/dist-packages/tornado-3.0.1-py2.7.egg/tornado/tcpserver.py", line 228, in handle_connection
self.handle_stream(stream, address)
File "/usr/local/lib/python2.7/dist-packages/tornado-3.0.1-py2.7.egg/tornado/httpserver.py", line 157, in handle_stream
self.no_keep_alive, self.xheaders, self.protocol)
File "/usr/local/lib/python2.7/dist-packages/tornado-3.0.1-py2.7.egg/tornado/httpserver.py", line 190, in __init_

self.stream.read_until(b"\r\n\r\n", self._header_callback)
File "/usr/local/lib/python2.7/dist-packages/tornado-3.0.1-py2.7.egg/tornado/iostream.py", line 148, in read_until
self._try_inline_read()
File "/usr/local/lib/python2.7/dist-packages/tornado-3.0.1-py2.7.egg/tornado/iostream.py", line 398, in _try_inline_read
if self._read_to_buffer() == 0:
File "/usr/local/lib/python2.7/dist-packages/tornado-3.0.1-py2.7.egg/tornado/iostream.py", line 432, in _read_to_buffer
raise IOError("Reached maximum read buffer size")
IOError: Reached maximum read buffer size
but after several big file upload request, there is a big memory increase in tornado server, so i think may be there is a memory leak in processing big file
any suggestions? how could i fix it up?

@schlamar

Try to set self._read_buffer = None in BaseIOStream.close.

A proper fix should clear the write buffer, too.

@wsantos

Should we dynamically increase max_buffer_size ?

        if self._read_buffer_size >= self.max_buffer_size:
            gen_log.error("Reached maximum read buffer size")
            self.close()
            raise IOError("Reached maximum read buffer size")
        return len(chunk)
@schlamar

@wsantos I think the issue is not that the IOError is thrown, which is expected behavior. (Am I right @JerryKwan?)

The unexpected issue is that the memory of the read buffer does not get freed after the IOStream is closed (I assume it doesn't get garbage collected). So the right thing to do would be to free the read (and write) buffer while closing the IOStream.

@schlamar

Increasing a max_* value is really arbitrary behavior, I strongly discourage that.

@wsantos

@schlamar, so its ok to have a buffer with size 104857600 (100M) and send 101M?, how the IOStream handle this ?

@schlamar

@JerryKwan BTW, for such large uploads I would patch tornado to buffer directly on disk.

@wsantos Raises the IOError as expected. On low-level you can handle this gracefully by processing portions of the data (e.g. with read_bytes). The HTTP part however does read_until(HTTP_END), so if the data sent is bigger than 100M (in one HTTP request of course) it will fail. This is IMO fine as you can either adjust the limit on your own or overcome this limitation completely by manually buffering on disk. The latter one would require some tough work, but is the most suitable in this case IMO.

@wsantos

@schlamar i agree, now we can start a server and set the limit we can handle look #732

@JerryKwan it's OK to you to have a configurable option on server start ? maybe it will be the best option.

@schlamar

FYI; bottle is automatically switching to a temporary file for file uploads if content length of request is bigger than max buffer size: https://github.com/defnull/bottle/blob/d89d676a6510541e679fac1ac1e8a5745eb87f76/bottle.py#L1050

Tornado should be doing the same IMO (and decrease max buffer size to 1M or less).

@wsantos

We need to look at this with care, it will block the tornado during file upload ?

@schlamar

We need to look at this with care, it will block the tornado during file upload ?

What exactly and why should any of the proposals here block?

@wsantos

When we are writing in tempfile.

@schlamar

When we are writing in tempfile.

Really? What do you think is blocking more serious: writing 100M all at once to a file (this is what a user usually does after a file upload) or write small chunks to a file...

@saghul

The unexpected issue is that the memory of the read buffer does not get freed after the IOStream is closed (I assume it doesn't get garbage collected). So the right thing to do would be to free the read (and write) buffer while closing the IOStream.

The problem is that someone may want to read the remaining data in the buffer. I guess that reading until you get an exception would consume the buffer and 'fix' the issue. Or maybe have a dedicated cleanup method.

@schlamar

I guess that reading until you get an exception would consume the buffer

It does already read until the buffer is full, then it closes the stream and throws the exception. The buffer doesn't get consumed at all if context length > max buffer size.

Why should anyone read the buffer after the stream is closed? A close forbids any further interaction with the stream anyhow (see _checked_close).

@saghul

Imagine I'm using read_until_delimiter and the connection gets closed. There is data in the buffer which wasn't consumed.

You can actually read that data, with read_bytes, the close check is done after attempting a read from the buffer: https://github.com/facebook/tornado/blob/master/tornado/iostream.py#L383

@wsantos
@schlamar

Sure. But this is IMO practically irrelevant. If you could consume the buffer with read_bytes, you would have done this in the first place and prevent the buffer overflow at all :)
If your buffer is full, you reach a point where the content of the buffer is not usable and can be thrown away. Otherwise you just designed your protocol wrong.

@schlamar

Or you allow the user to register a callback for buffer overflow and the default case (no callback registered) would be raising the IOError and clearing the buffer.

@wsantos
@schlamar

and create other for discuss the Tempfile when size is more than mx_buffer_size. what do you think

Exactly my thought :+1:

@wsantos

Issue for stream bigger than max_buffer_size #743, im making some testes if memory leak issue.

regards

@saghul

Sure. But this is IMO practically irrelevant. If you could consume the buffer with read_bytes, you would have done this in the first place and prevent the buffer overflow at all :)
If your buffer is full, you reach a point where the content of the buffer is not usable and can be thrown away. Otherwise you just designed your protocol wrong.

IIRC there are other cases when the connection is interrupted (like in case the remote closes it) and the buffer is not emptied either. I can happen for reasons other than overflow.

@wsantos wsantos referenced this issue from a commit
@wsantos wsantos fix #740 49e49b6
@wsantos wsantos referenced this issue from a commit
@wsantos wsantos Fix #740 2129ff4
@schlamar

IIRC there are other cases when the connection is interrupted (like in case the remote closes it) and the buffer is not emptied either. I can happen for reasons other than overflow.

This case should be covered here: https://github.com/facebook/tornado/blob/master/tornado/iostream.py#L289

@JerryKwan

sorry for the late reply
as discussed before, i think the buffered data should be cleared when some exception occured.
so when i add the following data clear statements in function close() in iostream.py, seems like the memory leak problem is solved
now the close() function looks like this:

def close(self, exc_info=False):
        """Close this stream.
        If ``exc_info`` is true, set the ``error`` attribute to the current
        exception from `sys.exc_info` (or if ``exc_info`` is a tuple,
        use that instead of `sys.exc_info`).
        """
        if not self.closed():
            if exc_info:
                if not isinstance(exc_info, tuple):
                    exc_info = sys.exc_info()
                if any(exc_info):
                    self.error = exc_info[1]
            if self._read_until_close:
                callback = self._read_callback
                self._read_callback = None
                self._read_until_close = False
                self._run_callback(callback,
                                   self._consume(self._read_buffer_size))
            if self._state is not None:
                self.io_loop.remove_handler(self.fileno())
                self._state = None
            self.close_fd()
            # clear unneeded buffered data
            self._read_buffer = None
            self._write_buffer = None
            self._closed = True

        self._maybe_run_close_callback()
@wsantos wsantos referenced this issue from a commit
@wsantos wsantos Fix #740
reorganize the code.
bdeeccb
@bdarnell
Owner

The IOStream, buffered data and all, should be getting garbage collected when it's no longer needed. What's keeping the IOStreams alive in this case?

@bdarnell bdarnell closed this issue from a commit
@bdarnell bdarnell Fix a memory leak in HTTPServer with very large file uploads.
This is not quite a true leak since the garbage collector will
reclaim everything when it runs, but it might as well be a leak
because CPython's garbage collector uses heuristics based on the
number of allocated objects to decide when to run.  Since this
scenario resulted in a small number of very large objects, the process
could consume a large amount of memory.

The actual change is to ensure that HTTPConnection always sets a
close_callback on the IOStream instead of waiting for the Application
to set one.  I also nulled out a few more references to break up cycles.

Closes #740.
Closes #747.
Closes #760.
12780c1
@bdarnell bdarnell closed this in 12780c1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.