Commit e9180de1 authored by Kirill Smelkov's avatar Kirill Smelkov

golang: pyselect: Fix tx object reference leak on error exit

Before passing objects to _chanselect for send, pyselect increfs them, as just
send does, to indicate that one object reference is passed to channel buffer.
On exit, since only one case is actually executed by select, pyselect needs to
decref incref'ed object from not executed cases.

Pyselect already implements the latter cleanup, but currently the cleanup is
executed only if control flow reaches _chanselect at all. Which is a bug, since
pyselect can panic or raise an exception just in the middle of preparation phase.

-> Fix it by associating the finally-decref cleanup with whole
prepare+_chanselect code.

Without the fix, the second part of added test (abnormal exit) fails e.g. like:

        @mark.skipif(not hasattr(sys, 'getrefcount'),   # skipped e.g. on PyPy
                     reason="needs sys.getrefcount")
        def test_select_refleak():
            ch1 = chan()
            ch2 = chan()
            obj1 = object()
            obj2 = object()
            tx1 = (ch1.send, obj1)
            tx2 = (ch2.send, obj2)

            # normal exit
            gc.collect()
            nref1 = sys.getrefcount(obj1)
            nref2 = sys.getrefcount(obj2)
            _, _rx = select(
                tx1,        # 0
                tx2,        # 1
                default,    # 2
            )
            assert (_, _rx) == (2, None)
            gc.collect()
            assert sys.getrefcount(obj1) == nref1
            gc.collect()
            assert sys.getrefcount(obj1) == nref2

            # abnormal exit
            with raises(AttributeError) as exc:
                select(
                    tx1,        # 0
                    tx2,        # 1
                    'zzz',      # 2 causes pyselect to panic
                )
            assert exc.value.args == ("'str' object has no attribute '__self__'",)
            gc.collect()
    >       assert sys.getrefcount(obj1) == nref1
    E       assert 4 == 3
    E         -4
    E         +3

    golang/golang_test.py:690: AssertionError

The bug was introduced in 3b241983 (Port/move channels to C/C++/Pyx).
parent f2847307
......@@ -272,60 +272,61 @@ pydefault = object()
# ...
def pyselect(*pycasev):
cdef int i, n = len(pycasev), selected
cdef vector[_selcase] casev = vector[_selcase](n)
cdef vector[_selcase] casev = vector[_selcase](n, default)
cdef pychan pych
cdef PyObject *_rx = NULL # all select recvs are setup to receive into _rx
cdef cbool rxok = False # (its ok as only one receive will be actually executed)
# prepare casev for chanselect
for i in range(n):
pycase = pycasev[i]
# default
if pycase is pydefault:
casev[i] = default
# send
elif type(pycase) is tuple:
if len(pycase) != 2:
pypanic("pyselect: invalid [%d]() case" % len(pycase))
_tcase = <PyTupleObject *>pycase
pysend = <object>(_tcase.ob_item[0])
if pysend.__self__.__class__ is not pychan:
pypanic("pyselect: send on non-chan: %r" % (pysend.__self__.__class__,))
pych = pysend.__self__
if pysend.__name__ != "send": # XXX better check PyCFunction directly
pypanic("pyselect: send expected: %r" % (pysend,))
# wire ptx through pycase[1]
p_tx = &(_tcase.ob_item[1])
tx = <object>(p_tx[0])
# incref tx as if corresponding channel is holding pointer to the object while it is being sent.
# we'll decref the object if it won't be sent.
# see pychan.send for details.
Py_INCREF(tx)
casev[i] = _selsend(pych._ch, p_tx)
# recv
else:
pyrecv = pycase
if pyrecv.__self__.__class__ is not pychan:
pypanic("pyselect: recv on non-chan: %r" % (pyrecv.__self__.__class__,))
pych = pyrecv.__self__
if pyrecv.__name__ == "recv": # XXX better check PyCFunction directly
casev[i] = _selrecv(pych._ch, &_rx)
elif pyrecv.__name__ == "recv_": # XXX better check PyCFunction directly
casev[i] = _selrecv_(pych._ch, &_rx, &rxok)
else:
pypanic("pyselect: recv expected: %r" % (pyrecv,))
selected = -1
try:
# prepare casev for chanselect
for i in range(n):
pycase = pycasev[i]
# default
if pycase is pydefault:
casev[i] = default
# send
elif type(pycase) is tuple:
if len(pycase) != 2:
pypanic("pyselect: invalid [%d]() case" % len(pycase))
_tcase = <PyTupleObject *>pycase
pysend = <object>(_tcase.ob_item[0])
if pysend.__self__.__class__ is not pychan:
pypanic("pyselect: send on non-chan: %r" % (pysend.__self__.__class__,))
pych = pysend.__self__
if pysend.__name__ != "send": # XXX better check PyCFunction directly
pypanic("pyselect: send expected: %r" % (pysend,))
# wire ptx through pycase[1]
p_tx = &(_tcase.ob_item[1])
tx = <object>(p_tx[0])
# incref tx as if corresponding channel is holding pointer to the object while it is being sent.
# we'll decref the object if it won't be sent.
# see pychan.send for details.
Py_INCREF(tx)
casev[i] = _selsend(pych._ch, p_tx)
# recv
else:
pyrecv = pycase
if pyrecv.__self__.__class__ is not pychan:
pypanic("pyselect: recv on non-chan: %r" % (pyrecv.__self__.__class__,))
pych = pyrecv.__self__
if pyrecv.__name__ == "recv": # XXX better check PyCFunction directly
casev[i] = _selrecv(pych._ch, &_rx)
elif pyrecv.__name__ == "recv_": # XXX better check PyCFunction directly
casev[i] = _selrecv_(pych._ch, &_rx, &rxok)
else:
pypanic("pyselect: recv expected: %r" % (pyrecv,))
with nogil:
selected = _chanselect_pyexc(&casev[0], casev.size())
finally:
# decref not sent tx (see ^^^ send prepare)
for i in range(n):
......
......@@ -22,7 +22,7 @@ from __future__ import print_function, absolute_import
from golang import go, chan, select, default, nilchan, _PanicError, func, panic, defer, recover
from golang import sync
from pytest import raises
from pytest import raises, mark
from os.path import dirname
import os, sys, inspect, importlib
from subprocess import Popen, PIPE
......@@ -652,6 +652,46 @@ def test_select():
assert len_sendq(ch2) == len_recvq(ch2) == 0
# verify that select does not leak references to passed objects.
@mark.skipif(not hasattr(sys, 'getrefcount'), # skipped e.g. on PyPy
reason="needs sys.getrefcount")
def test_select_refleak():
ch1 = chan()
ch2 = chan()
obj1 = object()
obj2 = object()
tx1 = (ch1.send, obj1)
tx2 = (ch2.send, obj2)
# normal exit
gc.collect()
nref1 = sys.getrefcount(obj1)
nref2 = sys.getrefcount(obj2)
_, _rx = select(
tx1, # 0
tx2, # 1
default, # 2
)
assert (_, _rx) == (2, None)
gc.collect()
assert sys.getrefcount(obj1) == nref1
gc.collect()
assert sys.getrefcount(obj1) == nref2
# abnormal exit
with raises(AttributeError) as exc:
select(
tx1, # 0
tx2, # 1
'zzz', # 2 causes pyselect to panic
)
assert exc.value.args == ("'str' object has no attribute '__self__'",)
gc.collect()
assert sys.getrefcount(obj1) == nref1
gc.collect()
assert sys.getrefcount(obj1) == nref2
# benchmark sync chan send vs recv on select side.
def bench_select(b):
ch1 = chan()
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment