PR#1653: Allow ClientSession objects to get cleaned up by the garbage collector - koji

koji

#1653 Allow ClientSession objects to get cleaned up by the garbage collector

Merged 4 years ago by mikem. Opened 4 years ago by mprahl.

Unknown source circular-ref into master

Allow ClientSession objects to get cleaned up by the garbage collector

mprahl • 4 years ago

a552a24

koji/__init__.py

file modified

+11 -2

		`@@ -77,6 +77,7 @@`
		`import time`
		`import traceback`
		`import warnings`
		`+ import weakref`
		`import xml.sax`
		`import xml.sax.handler`
		`import six.moves.urllib`
		`@@ -2141,7 +2142,10 @@`
		`self.opts = opts`
		`self.authtype = None`
		`self.setSession(sinfo)`
		`- self._multicall = MultiCallHack(self)`
		`+ # Use a weak reference here so the garbage collector can still clean up`
		`+ # ClientSession objects even with a circular reference, and the optional`
		`+ # cycle detector being disabled due to the __del__ method being used.`
		`+ self._multicall = MultiCallHack(weakref.ref(self))`
		`self._calls = []`
		`self.logger = logging.getLogger('koji')`
		`self.rsession = None`
		`@@ -2889,6 +2893,9 @@`

		`def __init__(self, session):`
		`self.value = False`
		`+ # session must be a weak reference`
		`+ if not isinstance(session, weakref.ReferenceType):`
		`+ raise TypeError('The session parameter must be a weak reference')`
		`self.session = session`

		`def __nonzero__(self):`
		`@@ -2898,7 +2905,9 @@`
		`return self.value`

		`def __call__(self, **kw):`
		`- return MultiCallSession(self.session, **kw)`
		`+ # self.session is a weak reference, which is why it is being called`
		`+ # first`
		`+ return MultiCallSession(self.session(), **kw)`


		`class MultiCallNotReady(Exception):`

tests/test_lib/test_client_session.py

file modified

		`@@ -1,6 +1,7 @@`
		`from __future__ import absolute_import`
		`import mock`
		`import six`
		`+ import weakref`
		`try:`
		`import unittest2 as unittest`
		`except ImportError:`
		`@@ -227,3 +228,11 @@`
		`self.assertEqual(2, self.ksession._sendCall.call_count)`
		`self.assertEqual([['a', 'b', 'c'],`
		`{'faultCode': 1000, 'faultString': 'msg'}], ret)`
		`+`
		`+ def test_MultiCallHack_weakref_validation(self):`
		`+ expected_exc = 'The session parameter must be a weak reference'`
		`+ with self.assertRaisesRegexp(TypeError, expected_exc):`
		`+ koji.MultiCallHack(self.ksession)`
		`+`
		`+ # This should not raise an exception`
		`+ koji.MultiCallHack(weakref.ref(self.ksession))`

mprahl commented 4 years ago

Fixes #1652

rebased onto 49446e1bf0f744a07d6d2d334d33a1c8a6a52cf1

4 years ago

rebased onto 630f8323debf11d0f504d39371e725c39110b00c

4 years ago

mikem commented 4 years ago

This looks mostly like the right thing, but I have a slight pause about the possibility of a MultiCallSession class outliving the session it is based on. I'm not sure if that's completely sane, but it's not completely unreasonable either.

Seems like while we need to avoid the circular reference, we still ought to have a real reference in MultiCallSession. Maybe something like this?

https://github.com/mikem23/koji-playground/commits/pagure/pr/1653

mikem commented 4 years ago

Here's a simple example

import koji

def get_multi():
    session = koji.ClientSession('https://koji.fedoraproject.org/kojihub')
    return session.multicall()

with get_multi() as m:
    builds = [m.getBuild(i) for i in range(5000,100000, 10000)]
builds = [b.result for b in builds]

Again, maybe not completely sane for the MultiCallSession to continue after the base session is unreferenced, but I don't necessarily see why it shouldn't work either.

rebased onto a552a24

4 years ago

mprahl commented 4 years ago

Thanks @mikem. How does this look?

mikem commented 4 years ago

:thumbsup:

mikem commented 4 years ago

Commit a5af69c fixes this pull-request