Commit 059c71e1 authored by Kirill Smelkov's avatar Kirill Smelkov

bigfile/zodb: Do not hold reference to ZBigFileH indefinitely in Connection.onOpenCallback

If we do - ZBigFileH objects just don't get garbage collected, and
sooner or later this way it leaks enough filedescriptors so that main
zope loop breaks:

    Traceback (most recent call last):
      File ".../bin/runzope", line 194, in <module>
        sys.exit(Zope2.Startup.run.run())
      File ".../eggs/Zope2-2.13.22-py2.7.egg/Zope2/Startup/run.py", line 26, in run
        starter.run()
      File ".../eggs/Zope2-2.13.22-py2.7.egg/Zope2/Startup/__init__.py", line 105, in run
        Lifetime.loop()
      File ".../eggs/Zope2-2.13.22-py2.7.egg/Lifetime/__init__.py", line 43, in loop
        lifetime_loop()
      File ".../eggs/Zope2-2.13.22-py2.7.egg/Lifetime/__init__.py", line 53, in lifetime_loop
        asyncore.poll(timeout, map)
      File ".../parts/python2.7/lib/python2.7/asyncore.py", line 145, in poll
        r, w, e = select.select(r, w, e, timeout)
    ValueError: filedescriptor out of range in select()

    $ lsof -p <runzope-pid> |grep ramh | wc -l
    950

So continuing 64d1f40b (bigfile/zodb: Monkey-patch for ZODB.Connection
to support callback on .open()) let's change the implementation to use
WeakSet for callbacks list.

Yes, because weakref to bound methods release immediately, we give up
flexibility to subscribe to arbitrary callbacks. If it become an issue,
something like WeakMethod from py3 or recipes from the net how to do it
are there.
parent 105ab1c7
...@@ -260,7 +260,7 @@ def Connection_onOpenCallback(self, f): ...@@ -260,7 +260,7 @@ def Connection_onOpenCallback(self, f):
if self._onOpenCallbacks is None: if self._onOpenCallbacks is None:
# NOTE WeakSet does not work for bound methods - the are always create # NOTE WeakSet does not work for bound methods - the are always create
# anew for each obj.method access, and thus will go away almost immediately # anew for each obj.method access, and thus will go away almost immediately
self._onOpenCallbacks = set() self._onOpenCallbacks = WeakSet()
self._onOpenCallbacks.add(f) self._onOpenCallbacks.add(f)
assert not hasattr(Connection, 'onOpenCallback') assert not hasattr(Connection, 'onOpenCallback')
...@@ -270,9 +270,12 @@ orig_Connection_open = Connection.open ...@@ -270,9 +270,12 @@ orig_Connection_open = Connection.open
def Connection_open(self, transaction_manager=None, delegate=True): def Connection_open(self, transaction_manager=None, delegate=True):
orig_Connection_open(self, transaction_manager, delegate) orig_Connection_open(self, transaction_manager, delegate)
# FIXME method name hardcoded. Better not do it and allow f to be general
# callable, but that does not work with bound method - see above.
# ( Something like WeakMethod from py3 could help )
if self._onOpenCallbacks: if self._onOpenCallbacks:
for f in self._onOpenCallbacks: for f in self._onOpenCallbacks:
f() f.on_connection_open()
Connection.open = Connection_open Connection.open = Connection_open
# ------------ # ------------
...@@ -332,7 +335,7 @@ class _ZBigFileH(object): ...@@ -332,7 +335,7 @@ class _ZBigFileH(object):
self.transaction_manager = zfile._p_jar.transaction_manager self.transaction_manager = zfile._p_jar.transaction_manager
# when connection will be reopened -> txn_manager.registerSynch(self) # when connection will be reopened -> txn_manager.registerSynch(self)
zfile._p_jar.onOpenCallback(self.on_connection_open) zfile._p_jar.onOpenCallback(self) # -> self.on_connection_open()
# when we are just initially created, the connection is already opened, # when we are just initially created, the connection is already opened,
# so manually compensate for it. # so manually compensate for it.
......
...@@ -25,6 +25,8 @@ from transaction import TransactionManager ...@@ -25,6 +25,8 @@ from transaction import TransactionManager
from numpy import ndarray, array_equal, uint8, zeros from numpy import ndarray, array_equal, uint8, zeros
from threading import Thread from threading import Thread
from six.moves import _thread from six.moves import _thread
import weakref
import gc
from pytest import raises from pytest import raises
from six.moves import range as xrange from six.moves import range as xrange
...@@ -539,3 +541,34 @@ def test_bigfile_filezodb_vs_cache_invalidation(): ...@@ -539,3 +541,34 @@ def test_bigfile_filezodb_vs_cache_invalidation():
conn2.close() conn2.close()
del conn2, root2 del conn2, root2
dbclose(root1) dbclose(root1)
# verify that fileh are garbage-collected after user free them
def test_bigfile_filezodb_fileh_gc():
root1= dbopen()
conn1= root1._p_jar
db = conn1.db()
root1['zfile4'] = f1 = ZBigFile(blksize)
transaction.commit()
fh1 = f1.fileh_open()
vma1 = fh1.mmap(0, 1)
wfh1 = weakref.ref(fh1)
assert wfh1() is fh1
conn1.close()
del vma1, fh1, f1, root1
conn2 = db.open()
root2 = conn2.root()
f2 = root2['zfile4']
fh2 = f2.fileh_open()
vma2 = fh2.mmap(0, 1)
gc.collect()
assert wfh1() is None # fh1 should be gone
del vma2, fh2, f2
dbclose(root2)
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