#1653 Allow ClientSession objects to get cleaned up by the garbage collector
Merged 2 years ago by mikem. Opened 2 years ago by mprahl.
Unknown source circular-ref  into  master

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):

@@ -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))

rebased onto 49446e1bf0f744a07d6d2d334d33a1c8a6a52cf1

2 years ago

rebased onto 630f8323debf11d0f504d39371e725c39110b00c

2 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

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

2 years ago

Thanks @mikem. How does this look?

Commit a5af69c fixes this pull-request

Pull-Request has been merged by mikem

2 years ago