Skip to content
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

Memory leak between io.BufferedWriter and socket.sendall #1318

Closed
damz opened this issue Nov 21, 2018 · 7 comments

Comments

Projects
None yet
3 participants
@damz
Copy link

commented Nov 21, 2018

  • gevent version: 1.3.7
  • Python version: cPython 2.7.15
  • Operating System: Fedora 29

Description:

This is a rather weird issue. In the environment above, this leaks:

import gevent.monkey ; gevent.monkey.patch_all()

import io
import socket

import gevent


class SocketFile(object):
    def __init__(self, sock):
        self._sock = sock

    def readable(self):
        return False

    def writable(self):
        return True

    def write(self, data):
        self._sock.sendall(data)
        return len(data)

    @property
    def closed(self):
        return False


one, two = socket.socketpair()

def _read():
    while True:
        buf = one.recv(4096)
        if buf == "":
            break

gevent.spawn(_read)

f = SocketFile(two)
bf = io.BufferedWriter(f)

while True:
    bf.write("test")
    bf.flush()

This is because self._sock.sendall ends up receiving a memoryview object built by io.BufferedWriter, and somehow that ends up leaking... but:

  • Replacing self._sock.sendall(data) by self._sock.sendall(data.tobytes()) fixes the leak
  • Just s.sendall(memoryview("test")) doesn't seem to leak, so it is not just about passing a memoryview to socket.sendall
@arcivanov

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2018

and somehow that ends up leaking

How do you know if it's actually leaking?

Replacing self._sock.sendall(data) by self._sock.sendall(data.tobytes()) fixes the leak

Also creates a huge allocation completely destroying the point of memoryview in the first place.

@jamadden

This comment has been minimized.

Copy link
Member

commented Nov 21, 2018

With this example, I can reproduce an accumulation of memoryview objects (a leak) in 2.7.15, but not 3.5+.

In the given example, bf.flush() in the loop causes SocketFile.write to be called with a different 4-byte memoryview on each iteration. When that gets to sock.sendall, it goes to gevent._socket2._get_memory(), which does this:

def _get_memory(data):
    try:
        mv = memoryview(data)
        if mv.shape:
            return mv
        # No shape, probably working with a ctypes object,
        # or something else exotic that supports the buffer interface
        return mv.tobytes()
    except TypeError:
        # fixes "python2.7 array.array doesn't support memoryview used in
        # gevent.socket.send" issue
        # (http://code.google.com/p/gevent/issues/detail?id=94)
        return buffer(data)

The memoryview is duplicated, it has a shape ((4L,)), and so it is returned. The copy-constructor for memoryview creates a new object pointing to the same underlying buffer.

If I elide the copy by adding if isinstance(data, memoryview) and data.shape: return data to the top of _get_memory(), the leak goes away.

If I completely take gevent.socket out of the picture by simply having SocketFile.write() do memoryview(data) and return, the leak comes back. Indeed, without even importing gevent, the leak exists:

from __future__ import print_function

import io
import objgraph

class File(object):
    def readable(self):
        return False

    def writable(self):
        return True

    def write(self, data):
        x = memoryview(data)
        return len(data)

    @property
    def closed(self):
        return False


f = File()
bf = io.BufferedWriter(f)

i = 0

while True:
    if i == 0 or i % 100 == 0:
        # This reports 100 new memoryview objects each time
        objgraph.show_growth()

    bf.write("test")
    bf.flush()
    i += 1

I believe we're looking at a bug in io.BufferedWriter and its internal management of buffers. (I also couldn't reproduce this simply using bytearray and memoryview at the Python level.)

@jamadden

This comment has been minimized.

Copy link
Member

commented Nov 21, 2018

I benchmarked the current approach ('now') vs the approach that adds if isinstance(data, memoryview) to the top of the loop ('inst'), given a memoryview object, a byte string, and a bytearray:

.....................
now: mv: Mean +- std dev: 287 ns +- 19 ns
.....................
now: bytes: Mean +- std dev: 265 ns +- 9 ns
.....................
now: bytearray: Mean +- std dev: 268 ns +- 13 ns
.....................
inst: mv: Mean +- std dev: 200 ns +- 8 ns
.....................
inst: bytes: Mean +- std dev: 431 ns +- 14 ns
.....................
inst: bytearray: Mean +- std dev: 434 ns +- 17 ns

For the case that a memoryview is passed, it's a win (duplicating the memoryview is more expensive than the instance check). But for all the other cases, the isinstance check adds substantial overhead. So I'd prefer not to have to go down that path.

I haven't been able to locate this exact issue on bugs.python.org, I'll keep trying, and probably wind up opening one.

@damz

This comment has been minimized.

Copy link
Author

commented Nov 21, 2018

@jamadden Thank you so much for looking into this. Just in case, I submitted #1319 which is the isinstance-based fix.

@jamadden

This comment has been minimized.

Copy link
Member

commented Nov 21, 2018

@jamadden

This comment has been minimized.

Copy link
Member

commented Nov 21, 2018

Using if type(data) is memoryview also solves this case, and has roughly half the penalty. You can't subclass memoryview, at least in pure-Python, so that's probably safe.

Benchmark current ininstance type
mv 269 ns 193 ns: 1.39x faster (-28%) 191 ns: 1.41x faster (-29%)
bytes 258 ns 406 ns: 1.57x slower (+57%) 340 ns: 1.32x slower (+32%)
bytearray 255 ns 411 ns: 1.61x slower (+61%) 343 ns: 1.34x slower (+34%)
@jamadden

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

It looks like if we cython-compile the function dealing with the memory view, we can make the isinstance version faster than the existing code (tested just now with CPython 2.7.16 and Cython 0.29.6).

Benchmark current cython isinstance
mv 340 ns 128 ns
bytes 326 ns 247 ns
bytearray 330 ns 244 ns

jamadden added a commit that referenced this issue Mar 22, 2019

Avoid a memory leak when wrapping an io.BufferedWriter around a socket.
Implement the fix in Cython so we don't lose any speed (we actually benchmark a bit faster).

Fixes #1318 and fixes #1319
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.