For anyone interested: I'm trying to debug a memory problem in the
ctypes-python bindings Array class which models an apr_array_header_t
array. I suppose this class may never have worked properly, as it gets
exercised very little within the bindings and had no unit test cases
until today.
Running within its test suite it mostly works, but there appears to be a
problem in making a deep copy of, or holding onto a reference to, each
element when a new array is initialized.
A simple demonstration in the Python interpreter:
$ (cd subversion/bindings/ctypes-python/ && python -i
test/setup_path.py)
>>> from csvn.core import *
>>> from csvn.types import Array
>>> a1 = Array(c_char_p, ['one'])
>>> a1
Array(['Array('])
The last line should have shown "Array(['one'])", but it printed garbage
from the Python interpreter's memory. This is so bad that I wonder if
my expectations are wrong.
The attached patch contains various changes and additions that I tried
unsuccessfully, and it adds tests that all pass (for me) except for the
places where they extend an Array with another (or the same) array.
Initially I thought the problem was only with self-referencing, because
I saw the failures in expressions such as "a1.extend(a1)", but now I am
noticing the same effect more widely.
Any hints appreciated.
- Julian
In the ctypes-python bindings, add more tests for the 'Array' class and make
some unsuccessful attempts to fix a self-referencing bug that hese find.
### Some of these tests fail and are named 'xfail_test_*' so that the normal
### test runner does not run them. (The ability to mark a test as Xfail
### requires either Python 2.7 or the 'unittest2' backport module.)
* subversion/bindings/ctypes-python/csvn/ext/listmixin.py
(ListMixin.__setitem__): If the new value is a self-reference then make a
new copy of it. According to
<http://code.activestate.com/recipes/440656-list-mixin/#c5>, this is
needed to handle cases where a list is inserted into itself.
### This doesn't seem to make any difference.
(test_list_mixin): Greatly cut down the set of initial list lengths to
test, in order to reduce the test run time from a few minutes to a few
seconds.
* subversion/bindings/ctypes-python/csvn/types.py
(Array._resize_region): Try a couple of alternative implementations. My
version of the 'memmove' code is what I would like to think is the
correct code to open up a space for more elements, where the existing
code looked plain wrong to me. The plain Python code is useful for
running the ListMixin self-test code, because it works with Array(c_int)
(and indeed that test then passes), whereas the code using 'byref'
throws a TypeError assertion in that case.
(Array._constructor): New method; the ListMixin documentation says it is
required.
### It doesn't seem to be required in practice, or at least adding or
removing this implementation seems to make no difference.
(Array): Add assertions as found in the 'ListMixin' example code.
* subversion/bindings/ctypes-python/test/svntypes.py
(ArrayTestCase): Add three new tests.
(__main__): Run just the 'xfail_*' tests.
### Temporary.
--This line, and those below, will be ignored--
Index: subversion/bindings/ctypes-python/csvn/ext/listmixin.py
===================================================================
--- subversion/bindings/ctypes-python/csvn/ext/listmixin.py (revision 1157788)
+++ subversion/bindings/ctypes-python/csvn/ext/listmixin.py (working copy)
@@ -132,6 +132,8 @@ class ListMixin(object):
return self._get_element(self._fix_index(i))
def __setitem__(self, i, value):
+ if value is self:
+ value = self._constructor(value)
if isinstance(i, slice):
(start, end, step) = self._tuple_from_slice(i)
if step != None:
@@ -389,8 +391,8 @@ def test_list_mixin(list_class=TestList,
import random
import sys
- for i in [1, 3, 8, 16, 18, 29, 59, 111, 213, 501, 1013,
- 2021, 3122, 4039, 5054]:
+ for i in [1, 3, 8, 16, 18, 29]: #, 59, 111, 213, 501, 1013,
+ #2021, 3122, 4039, 5054]:
x = [rand_elem() for j in range(i)]
y = list_class(x)
Index: subversion/bindings/ctypes-python/csvn/types.py
===================================================================
--- subversion/bindings/ctypes-python/csvn/types.py (revision 1157788)
+++ subversion/bindings/ctypes-python/csvn/types.py (working copy)
@@ -22,7 +22,7 @@ from UserDict import DictMixin
from shutil import copyfileobj
from tempfile import TemporaryFile
-# This class contains Pythonic wrappers for generic Subversion and
+# This module contains Pythonic wrappers for generic Subversion and
# APR datatypes (e.g. dates, streams, hashes, arrays, etc).
#
# These wrappers are used by higher-level APIs in csvn to wrap
@@ -140,16 +140,25 @@ class Array(ListMixin):
elts = property(fget=lambda self: cast(self.header[0].elts.raw,
POINTER(self.type)))
+ def _constructor(self, iterable):
+ return Array(self.type, iterable)
+
def _get_element(self, i):
+ assert 0 <= i < len(self)
return self.elts[i]
def _set_element(self, i, value):
+ assert 0 <= i < len(self)
self.elts[i] = value
def __len__(self):
return self.header[0].nelts
def _resize_region(self, start, end, new_size):
+ assert 0 <= start <= len(self)
+ assert 0 <= end <= len(self)
+ assert start <= end
+
diff = start-end+new_size
# Growing
@@ -161,20 +170,27 @@ class Array(ListMixin):
apr_array_push(self)
# Move the old items out of the way, if necessary
- if end < l:
- src_idx = max(end-diff,0)
- memmove(byref(self.elts + end),
- byref(self.elts[src_idx]),
- (l-src_idx)*self.header[0].elt_size)
+ try:
+ if end < l:
+ memmove(byref(self.elts[end+diff]),
+ byref(self.elts[end]),
+ (l-end)*self.header[0].elt_size)
+ except TypeError:
+ for i in range(l - end):
+ self.elts[l + diff - 1 - i] = self.elts[l - 1 - i]
# Shrinking
elif diff < 0:
# Overwrite the deleted items with items we still need
- if end < len(self):
+ try:
+ if end < len(self):
memmove(byref(self.elts[end+diff]),
byref(self.elts[end]),
(len(self)-end)*self.header[0].elt_size)
+ except TypeError:
+ for i in range(len(self) - end):
+ self.elts[end + diff + i] = self.elts[end + i]
# Shrink the array
for i in range(-diff):
Index: subversion/bindings/ctypes-python/test/svntypes.py
===================================================================
--- subversion/bindings/ctypes-python/test/svntypes.py (revision 1157788)
+++ subversion/bindings/ctypes-python/test/svntypes.py (working copy)
@@ -79,6 +79,13 @@ class HashTestCase(unittest.TestCase):
class ArrayTestCase(unittest.TestCase):
+ def xfail_test_append_self(self):
+ a = Array(c_char_p, ['one', 'two'])
+ import pdb
+ pdb.set_trace()
+ a.extend(a)
+ self.assertEqual(a, ['one', 'two', 'one', 'two'])
+
def test_array(self):
self.pyarray = ["vini", "vidi", "vici"]
self.svnarray = _types.Array(c_char_p, self.pyarray)
@@ -86,6 +93,58 @@ class ArrayTestCase(unittest.TestCase):
self.assertEqual(self.svnarray[2], "vici")
self.assertEqual(self.svnarray[1], "vidi")
+ def test_mixin_int(self):
+ # Run the tests provided by the Array's base class.
+ from csvn.ext.listmixin import test_list_mixin
+ import random
+
+ # The 'list_mixin' test assumes an array class that implements an
+ # array of a specific type, so create such a class based on Array.
+ class ArrayOfInt(Array):
+ def __init__(self, items):
+ Array.__init__(self, c_int, items)
+ def _constructor(self, iterable):
+ return ArrayOfInt(iterable)
+
+ def rand_elem():
+ return random.randrange(100)
+
+ test_list_mixin(ArrayOfInt, rand_elem)
+
+ def xfail_test_mixin_str(self):
+ # Run the tests provided by the Array's base class.
+ from csvn.ext.listmixin import test_list_mixin
+ import random
+
+ # The 'list_mixin' test assumes an array class that implements an
+ # array of a specific type, so create such a class based on Array.
+ class ArrayOfStr(Array):
+ def __init__(self, items):
+ Array.__init__(self, c_char_p, items)
+ def _constructor(self, iterable):
+ return ArrayOfStr(iterable)
+
+ def rand_elem():
+ return 'foo:' + str(random.randrange(100))
+
+ test_list_mixin(ArrayOfStr, rand_elem)
+
+ def xfail_test_extend(self):
+ a = Array(c_char_p)
+ a.extend(['a0', 'a1'])
+ a += ['a2']
+ self.assertEqual(a, ['a0', 'a1', 'a2'])
+ b = Array(c_char_p, a)
+ b[0] = 'b0new'
+ self.assertEqual(b, ['b0new', 'a1', 'a2'])
+ a.extend(b)
+ self.assertEqual(a, ['a0', 'a1', 'a2', 'b0new', 'a1', 'a2'])
+ a[0] = 'a0new'
+ a[4] = 'a4new'
+ b[2] = 'b2new'
+ self.assertEqual(a, ['a0new', 'a1', 'a2', 'b0new', 'a4new', 'a2'])
+ self.assertEqual(b, ['b0new', 'b1', 'b2new'])
+
def suite():
suite = unittest.TestSuite()
suite.addTest(unittest.makeSuite(SvnDateTestCase, 'test'))
@@ -94,5 +153,6 @@ def suite():
return suite
if __name__ == '__main__':
- runner = unittest.TextTestRunner()
- runner.run(suite())
+ runner = unittest.TextTestRunner(verbosity=2)
+ #runner.run(suite())
+ runner.run(unittest.makeSuite(ArrayTestCase, 'xfail_test'))
