From faf80b950f4b83f002eddf54403e14700f725478 Mon Sep 17 00:00:00 2001 From: Kevin Benton Date: Wed, 11 Jan 2017 17:38:30 -0800 Subject: [PATCH] Use weakrefs for common_db_mixin callbacks This will prevent the common_db_mixin dictionary extension functions and query hooks from stopping the GC of plugins in tests and causing resource leaks. Change-Id: I7576851a44abd14cbc337a3d3e28690c7316ec81 --- neutron/common/utils.py | 15 +++++++++++++ neutron/db/common_db_mixin.py | 36 +++++++++++++++++++----------- neutron/services/tag/tag_plugin.py | 21 ++++++++++------- neutron/tests/tools.py | 2 +- requirements.txt | 1 + 5 files changed, 53 insertions(+), 22 deletions(-) diff --git a/neutron/common/utils.py b/neutron/common/utils.py index 5bb069fccde..8ba28e4422d 100644 --- a/neutron/common/utils.py +++ b/neutron/common/utils.py @@ -27,6 +27,7 @@ import signal import sys import time import uuid +import weakref import debtcollector from debtcollector import removals @@ -877,3 +878,17 @@ def get_related_rand_names(prefixes, max_length=None): def get_related_rand_device_names(prefixes): return get_related_rand_names(prefixes, max_length=n_const.DEVICE_NAME_MAX_LEN) + + +try: + # PY3 + weak_method = weakref.WeakMethod +except AttributeError: + # PY2 + import weakrefmethod + weak_method = weakrefmethod.WeakMethod + + +def make_weak_ref(f): + """Make a weak reference to a function accounting for bound methods.""" + return weak_method(f) if hasattr(f, '__self__') else weakref.ref(f) diff --git a/neutron/db/common_db_mixin.py b/neutron/db/common_db_mixin.py index 4513b8e4fe0..efd676ddf3c 100644 --- a/neutron/db/common_db_mixin.py +++ b/neutron/db/common_db_mixin.py @@ -24,6 +24,7 @@ from sqlalchemy import or_ from sqlalchemy import sql from neutron.api.v2 import attributes +from neutron.common import utils from neutron.db import _utils as ndb_utils @@ -65,12 +66,20 @@ class CommonDbMixin(object): Filter hooks take as input the filter expression being built and return a transformed filter expression """ + if callable(query_hook): + query_hook = utils.make_weak_ref(query_hook) + if callable(filter_hook): + filter_hook = utils.make_weak_ref(filter_hook) + if callable(result_filters): + result_filters = utils.make_weak_ref(result_filters) cls._model_query_hooks.setdefault(model, {})[name] = { 'query': query_hook, 'filter': filter_hook, 'result_filters': result_filters} @classmethod def register_dict_extend_funcs(cls, resource, funcs): + funcs = [utils.make_weak_ref(f) if callable(f) else f + for f in funcs] cls._dict_extend_functions.setdefault(resource, []).extend(funcs) @property @@ -108,15 +117,11 @@ class CommonDbMixin(object): # Execute query hooks registered from mixins and plugins for _name, hooks in six.iteritems(self._model_query_hooks.get(model, {})): - query_hook = hooks.get('query') - if isinstance(query_hook, six.string_types): - query_hook = getattr(self, query_hook, None) + query_hook = self._resolve_ref(hooks.get('query')) if query_hook: query = query_hook(context, model, query) - filter_hook = hooks.get('filter') - if isinstance(filter_hook, six.string_types): - filter_hook = getattr(self, filter_hook, None) + filter_hook = self._resolve_ref(hooks.get('filter')) if filter_hook: query_filter = filter_hook(context, model, query_filter) @@ -192,24 +197,29 @@ class CommonDbMixin(object): query = query.filter(is_shared) for _nam, hooks in six.iteritems(self._model_query_hooks.get(model, {})): - result_filter = hooks.get('result_filters', None) - if isinstance(result_filter, six.string_types): - result_filter = getattr(self, result_filter, None) - + result_filter = self._resolve_ref( + hooks.get('result_filters', None)) if result_filter: query = result_filter(query, filters) return query + def _resolve_ref(self, ref): + """Finds string ref functions, handles dereference of weakref.""" + if isinstance(ref, six.string_types): + ref = getattr(self, ref, None) + if isinstance(ref, weakref.ref): + ref = ref() + return ref + def _apply_dict_extend_functions(self, resource_type, response, db_object): for func in self._dict_extend_functions.get( resource_type, []): args = (response, db_object) - if isinstance(func, six.string_types): - func = getattr(self, func, None) - else: + if not isinstance(func, six.string_types): # must call unbound method - use self as 1st argument args = (self,) + args + func = self._resolve_ref(func) if func: func(*args) diff --git a/neutron/services/tag/tag_plugin.py b/neutron/services/tag/tag_plugin.py index e4c4ca7e2df..9b1c0cb6ec6 100644 --- a/neutron/services/tag/tag_plugin.py +++ b/neutron/services/tag/tag_plugin.py @@ -118,11 +118,16 @@ class TagPlugin(common_db_mixin.CommonDbMixin, tag_ext.TagPluginBase): except exc.NoResultFound: raise tag_ext.TagNotFound(tag=tag) - # support only _apply_dict_extend_functions supported resources - # at the moment. - for resource, model in resource_model_map.items(): - common_db_mixin.CommonDbMixin.register_dict_extend_funcs( - resource, [_extend_tags_dict]) - common_db_mixin.CommonDbMixin.register_model_query_hook( - model, "tag", None, None, - functools.partial(tag_methods.apply_tag_filters, model)) + def __new__(cls, *args, **kwargs): + inst = super(TagPlugin, cls).__new__(cls, *args, **kwargs) + inst._filter_methods = [] # prevent GC of our partial functions + # support only _apply_dict_extend_functions supported resources + # at the moment. + for resource, model in resource_model_map.items(): + common_db_mixin.CommonDbMixin.register_dict_extend_funcs( + resource, [_extend_tags_dict]) + method = functools.partial(tag_methods.apply_tag_filters, model) + inst._filter_methods.append(method) + common_db_mixin.CommonDbMixin.register_model_query_hook( + model, "tag", None, None, method) + return inst diff --git a/neutron/tests/tools.py b/neutron/tests/tools.py index 6ffdcf082cc..ca1ff4a758d 100644 --- a/neutron/tests/tools.py +++ b/neutron/tests/tools.py @@ -125,7 +125,7 @@ class CommonDbMixinHooksFixture(fixtures.Fixture): def _setUp(self): self.original_hooks = common_db_mixin.CommonDbMixin._model_query_hooks self.addCleanup(self.restore_hooks) - common_db_mixin.CommonDbMixin._model_query_hooks = copy.deepcopy( + common_db_mixin.CommonDbMixin._model_query_hooks = copy.copy( common_db_mixin.CommonDbMixin._model_query_hooks) def restore_hooks(self): diff --git a/requirements.txt b/requirements.txt index b80890f0b1a..fba28080380 100644 --- a/requirements.txt +++ b/requirements.txt @@ -46,6 +46,7 @@ oslo.versionedobjects>=1.17.0 # Apache-2.0 osprofiler>=1.4.0 # Apache-2.0 ovs>=2.6.1 # Apache-2.0 pyroute2>=0.4.12 # Apache-2.0 (+ dual licensed GPL2) +weakrefmethod>=1.0.2;python_version=='2.7' # PSF python-novaclient!=2.33.0,>=2.29.0 # Apache-2.0 python-designateclient>=1.5.0 # Apache-2.0