Fix issue 740, memory leak #747

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
3 participants
@wsantos
Contributor

wsantos commented Apr 18, 2013

Fix issue #740

@schlamar

This comment has been minimized.

Show comment Hide comment
@schlamar

schlamar Apr 19, 2013

Contributor

Ok, in this case (I refer to the Travis results) I guess @saghul was right in his comment.

Please try to reset the buffer only in case of overflow (here). And use None instead as @JerryKwan mentioned, this is more elegant and makes the intention ("the buffer is invalid, don't use it anymore") more clear.

Contributor

schlamar commented Apr 19, 2013

Ok, in this case (I refer to the Travis results) I guess @saghul was right in his comment.

Please try to reset the buffer only in case of overflow (here). And use None instead as @JerryKwan mentioned, this is more elegant and makes the intention ("the buffer is invalid, don't use it anymore") more clear.

@wsantos

This comment has been minimized.

Show comment Hide comment
@wsantos

wsantos Apr 19, 2013

Contributor

I will rewrite, in the first fix i put the code before raise, i don't remember why i change this to close(), and the set None is the way to gom i dont know why i think in reausable IOStream.

Contributor

wsantos commented Apr 19, 2013

I will rewrite, in the first fix i put the code before raise, i don't remember why i change this to close(), and the set None is the way to gom i dont know why i think in reausable IOStream.

@schlamar

This comment has been minimized.

Show comment Hide comment
@schlamar

schlamar Apr 19, 2013

Contributor

That looks better 👍
I would suggest squashing the commits into one, having two commits for this PR adds just unnecessary noise to the history.

Contributor

schlamar commented Apr 19, 2013

That looks better 👍
I would suggest squashing the commits into one, having two commits for this PR adds just unnecessary noise to the history.

@bdarnell

This comment has been minimized.

Show comment Hide comment
@bdarnell

bdarnell Apr 24, 2013

Member

I don't think reaching the max_buffer_size should be treated differently from other errors; any remaining buffered data should be discarded once it is no longer needed or usable. It should probably be cleared in _maybe_run_close_callback, since that's where the real finalization of an IOStream occurs, but that causes some tests to fail. In some cases it looks like the tests may be in the wrong (they shouldn't rely on being able to read after the close callback has been run), but in other cases it looks like the IOStream is shutting down too soon and the tests aren't able to do a sequence of reads after the close. Maybe we need to introduce an explicit half-close to let the client indicate when it is done with the buffered data.

Member

bdarnell commented Apr 24, 2013

I don't think reaching the max_buffer_size should be treated differently from other errors; any remaining buffered data should be discarded once it is no longer needed or usable. It should probably be cleared in _maybe_run_close_callback, since that's where the real finalization of an IOStream occurs, but that causes some tests to fail. In some cases it looks like the tests may be in the wrong (they shouldn't rely on being able to read after the close callback has been run), but in other cases it looks like the IOStream is shutting down too soon and the tests aren't able to do a sequence of reads after the close. Maybe we need to introduce an explicit half-close to let the client indicate when it is done with the buffered data.

@wsantos

This comment has been minimized.

Show comment Hide comment
@wsantos

wsantos Apr 25, 2013

Contributor

Well, there is no memory leak at all, the problem is HTTPConnection stau instanced because the no_keep_alive, so it ok, the problem is the buffer not cleaned inside the HTTPConnection,so the fix was correct and must be placed in _maybe_run_close_callback, but with that tests now fail, @bdarnell can you explain the test:

  • test_read_until_close_after_close
  • test_streaming_read_until_close_after_close
    def test_read_until_close_after_close(self):
        # Similar to test_delayed_close_callback, but read_until_close takes
        # a separate code path so test it separately.
        server, client = self.make_iostream_pair()
        client.set_close_callback(self.stop)
        try:
            server.write(b"1234")
            server.close()
            self.wait()
            client.read_until_close(self.stop)
            data = self.wait()
            self.assertEqual(data, b"1234")
        finally:
            server.close()
            client.close()

Now with server.close() we clean the buffer so client can`t read, but i dont get what this test test, we are able to read buffer after close, is that a normal function ?

regards.

Contributor

wsantos commented Apr 25, 2013

Well, there is no memory leak at all, the problem is HTTPConnection stau instanced because the no_keep_alive, so it ok, the problem is the buffer not cleaned inside the HTTPConnection,so the fix was correct and must be placed in _maybe_run_close_callback, but with that tests now fail, @bdarnell can you explain the test:

  • test_read_until_close_after_close
  • test_streaming_read_until_close_after_close
    def test_read_until_close_after_close(self):
        # Similar to test_delayed_close_callback, but read_until_close takes
        # a separate code path so test it separately.
        server, client = self.make_iostream_pair()
        client.set_close_callback(self.stop)
        try:
            server.write(b"1234")
            server.close()
            self.wait()
            client.read_until_close(self.stop)
            data = self.wait()
            self.assertEqual(data, b"1234")
        finally:
            server.close()
            client.close()

Now with server.close() we clean the buffer so client can`t read, but i dont get what this test test, we are able to read buffer after close, is that a normal function ?

regards.

@bdarnell

This comment has been minimized.

Show comment Hide comment
@bdarnell

bdarnell Apr 26, 2013

Member

Keep-alive or not, the HTTPConnection should go away once the IOStream is closed. Is there still a chain of references keeping these objects alive, or perhaps another cycle that needs to be broken up to facilitate cpython's GC? The objgraph package is useful here.

The _after_close tests refer to the fact that a server can send some data and then close its side of the connection, while the client can continue to read from its side. In IOStream we delay the close callback as long as there is a continuous chain of reads (i.e. one read callback starts the next one). In these tests the chain is not actually continuous - we do a read, then some other things, then another read. That means that the close callback gets run before the client is done with the data. That wasn't a problem for these tests before since they don't use the close callback, but if we do additional cleanup at the same time as the close callback it exposes that things weren't quite right before.

I think we need to replace the pending_callbacks check in maybe_run_close_callback with something a little smarter. For example, if there is data in the read buffer and there is no read_callback, we should leave the stream open for reading (if there is a read_callback but there is not enough data in the buffer to satisfy the conditions of the read, we can close the stream down)

Member

bdarnell commented Apr 26, 2013

Keep-alive or not, the HTTPConnection should go away once the IOStream is closed. Is there still a chain of references keeping these objects alive, or perhaps another cycle that needs to be broken up to facilitate cpython's GC? The objgraph package is useful here.

The _after_close tests refer to the fact that a server can send some data and then close its side of the connection, while the client can continue to read from its side. In IOStream we delay the close callback as long as there is a continuous chain of reads (i.e. one read callback starts the next one). In these tests the chain is not actually continuous - we do a read, then some other things, then another read. That means that the close callback gets run before the client is done with the data. That wasn't a problem for these tests before since they don't use the close callback, but if we do additional cleanup at the same time as the close callback it exposes that things weren't quite right before.

I think we need to replace the pending_callbacks check in maybe_run_close_callback with something a little smarter. For example, if there is data in the read buffer and there is no read_callback, we should leave the stream open for reading (if there is a read_callback but there is not enough data in the buffer to satisfy the conditions of the read, we can close the stream down)

@bdarnell bdarnell closed this in 12780c1 Apr 29, 2013

@wsantos wsantos deleted the wsantos:fix740 branch Apr 29, 2013

@wsantos

This comment has been minimized.

Show comment Hide comment
@wsantos

wsantos Apr 29, 2013

Contributor

@bdarnell, Well, when tornado reache maximum read buffer size, the close() is called, but _maybe_run_close_callback dont run close_callback and dont cleanup variables

self.closed() = True
self._close_callback = wrapped function
self._pending_callbacks = 1

so the HTTPConnection keep the 100M data, until another request use the connection, in this case should i set _pending_callbacks to 0 so tornado made all cleanups ?

Contributor

wsantos commented Apr 29, 2013

@bdarnell, Well, when tornado reache maximum read buffer size, the close() is called, but _maybe_run_close_callback dont run close_callback and dont cleanup variables

self.closed() = True
self._close_callback = wrapped function
self._pending_callbacks = 1

so the HTTPConnection keep the 100M data, until another request use the connection, in this case should i set _pending_callbacks to 0 so tornado made all cleanups ?

@bdarnell

This comment has been minimized.

Show comment Hide comment
@bdarnell

bdarnell May 1, 2013

Member

Ah, interesting. I wonder why I didn't run into this in my testing. I think every time we do "self._pending_callbacks -= 1" we need to call _maybe_run_close_callback (or _maybe_add_error_listener?)

Member

bdarnell commented May 1, 2013

Ah, interesting. I wonder why I didn't run into this in my testing. I think every time we do "self._pending_callbacks -= 1" we need to call _maybe_run_close_callback (or _maybe_add_error_listener?)

@bdarnell

This comment has been minimized.

Show comment Hide comment
@bdarnell

bdarnell May 1, 2013

Member

It looks like a difference between _handle_read and _try_inline_read - if the error happens in _try_inline_read we can fail without ever reaching the close callback. I've added a check in commit 5cb8542 which should take care of this case.

Member

bdarnell commented May 1, 2013

It looks like a difference between _handle_read and _try_inline_read - if the error happens in _try_inline_read we can fail without ever reaching the close callback. I've added a check in commit 5cb8542 which should take care of this case.

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