zopefoundation / persistent

automatic persistence for Python objects
https://pypi.org/project/persistent/
Other
46 stars 28 forks source link

Python/C inconsistency: Cannot store raw `Persistent` objects in cache #133

Closed jamadden closed 4 years ago

jamadden commented 4 years ago

The C implementation lets you store basic Persistent instances in the cache:

>>> from persistent import Persistent
>>> from persistent import PickleCache
>>> obj = Persistent()
>>> type(obj)
<class 'persistent.Persistent'>
>>> key = obj._p_oid = b'00000000'
>>> obj._p_jar = object()
>>> cache[key] = obj
>>> cache[key]
<persistent.Persistent object at 0x1053ae960 oid 0x3030303030303030 in <object object at 0x1053a8430>>

But the pure-Python implementation does not:

>>> from persistent.picklecache import PickleCachePy
>>> from persistent import PersistentPy
>>> pyobj = PersistentPy()
>>> pycache = PickleCachePy(None, 10000)
>>> pyobj._p_jar = object()
>>> key = pyobj._p_oid = b'00000000'
>>> pycache[key] = pyobj
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "//persistent/persistent/picklecache.py", line 154, in __setitem__
    self.data[oid] = value
  File "//lib/python3.7/weakref.py", line 168, in __setitem__
    self.data[key] = KeyedRef(value, self._remove, key)
  File "//lib/python3.7/weakref.py", line 337, in __new__
    self = ref.__new__(type, ob, callback)
TypeError: cannot create weak reference to 'PersistentPy' object

This creates a test failure in ZODB:

$ PURE_PYTHON=1 zope-testrunner —test-path=src 
Failure in test doctest_invalidate (ZODB.tests.testConnection.InvalidationTests)
Failed doctest test for ZODB.tests.testConnection.InvalidationTests.doctest_invalidate
  File "/ZODB/src/ZODB/tests/testConnection.py", line 516, in doctest_invalidate

----------------------------------------------------------------------
File "/ZODB/src/ZODB/tests/testConnection.py", line 532, in ZODB.tests.testConnection.InvalidationTests.doctest_invalidate
Failed example:
    transaction.commit()
Exception raised:
    Traceback (most recent call last):
      File "//lib/python3.7/doctest.py", line 1329, in __run
        compileflags, 1), test.globs)
      File "<doctest ZODB.tests.testConnection.InvalidationTests.doctest_invalidate[9]>", line 1, in <module>
        transaction.commit()
      File "/transaction/transaction/_manager.py", line 257, in commit
        return self.manager.commit()
      File "/transaction/transaction/_manager.py", line 135, in commit
        return self.get().commit()
      File "/transaction/transaction/_transaction.py", line 282, in commit
        reraise(t, v, tb)
      File "/transaction/transaction/_compat.py", line 45, in reraise
        raise value
      File "/transaction/transaction/_transaction.py", line 273, in commit
        self._commitResources()
      File "/transaction/transaction/_transaction.py", line 465, in _commitResources
        reraise(t, v, tb)
      File "/transaction/transaction/_compat.py", line 45, in reraise
        raise value
      File "/transaction/transaction/_transaction.py", line 439, in _commitResources
        rm.commit(self)
      File "/ZODB/src/ZODB/Connection.py", line 497, in commit
        self._commit(transaction)
      File "/ZODB/src/ZODB/Connection.py", line 546, in _commit
        self._store_objects(ObjectWriter(obj), transaction)
      File "/ZODB/src/ZODB/Connection.py", line 609, in _store_objects
        self._cache[oid] = obj
      File "/persistent/persistent/picklecache.py", line 144, in __setitem__
        self.data[oid] = value
      File "///lib/python3.7/weakref.py", line 168, in __setitem__
        self.data[key] = KeyedRef(value, self._remove, key)
      File "///lib/python3.7/weakref.py", line 337, in __new__
        self = ref.__new__(type, ob, callback)
    TypeError: cannot create weak reference to 'Persistent' object

(Why don't we see this failure running ZODB with PyPy? I'm not sure, but I'd guess that we don't even try to run this test; for some reason, PyPy runs only 814 ZODB unit tests while CPython runs 1185. In total, PyPy runs 1313 tests while CPython runs 1684 tests.)

PythonLinks commented 4 years ago

Thank you for the excellent overview of the current problem. I have no idea what the cause of the problem is, nor how to fix it. But I do know one thing. Often just discussing a problem with others helps.

Maybe you could you be so kind as to explain how you think how this works, and what you have checked so far. By explaining your mental model of the system, one of the erudite developers on this mailing list may be able to point out what you are missing.

Chris

mgedmin commented 4 years ago

IIRC classes with __slots__ don't support weakrefs unless you explicitly add a slot for __weakref__. Would that be enough to fix this?

jamadden commented 4 years ago

IIRC classes with __slots__ don't support weakrefs unless you explicitly add a slot for __weakref__.

Right. Except, it turns out, on PyPy. That's why the test passes there.

>>>> class Foo(object):
....     __slots__ = ()
....
>>>> import weakref
>>>> weakref.ref(Foo())
<weakref at 0x000000010ff79500; to 'Foo'>

Would that be enough to fix this?

I think so, yes, at the cost of (1) increased size for all instances and (2) introducing a different incompatibility: CPersistent objects cannot be weakly referenced. In practice that's probably not very important, and since the main production use of the pure-Python implementation should be on PyPy (CPython+pure-Python is best for debugging and testing) where they can already be weakly referenced one could argue that it's even slightly more consistent, if one squints at it just right.

At any rate, I haven't thought of any other solution yet either.

mgedmin commented 4 years ago

How come CPersistent objects do not get the "TypeError: cannot create weak reference to ..." error then, if they don't support weakrefs?

jamadden commented 4 years ago

Because the C cache doesn’t use weakrefs. At least, not at the Python level. Internally it cheats the ref counting rules, and arranges for the C Persistent class to directly notify it when an instance is deallocated.