From acd27fb56c0a22b05f32dfe06cbb5c45509a1575 Mon Sep 17 00:00:00 2001 From: Emmanuel Garette Date: Tue, 27 Aug 2013 11:39:32 +0200 Subject: [PATCH] huge use of weakrefs to remove memoryleaks due to circular references --- test/test_storage.py | 10 ++-- tiramisu/config.py | 45 +++++++++--------- tiramisu/setting.py | 13 +++--- tiramisu/storage/dictionary/storage.py | 5 +- tiramisu/value.py | 65 ++++++++++++++------------ 5 files changed, 73 insertions(+), 65 deletions(-) diff --git a/test/test_storage.py b/test/test_storage.py index 44ba9d3..3857fb5 100644 --- a/test/test_storage.py +++ b/test/test_storage.py @@ -54,9 +54,9 @@ def test_delete_session_persistent(): pass else: from tiramisu.setting import list_sessions, delete_session - assert 'test_persistent' in list_sessions + assert 'test_persistent' in list_sessions() delete_session('test_persistent') - assert 'test_persistent' not in list_sessions + assert 'test_persistent' not in list_sessions() def test_create_persistent_retrieve(): @@ -68,14 +68,14 @@ def test_create_persistent_retrieve(): # storage is not persistent pass else: - assert c.b is False + assert c.b is None c.b = True assert c.b is True del(c) c = Config(o, session_id='test_persistent', persistent=True) assert c.b is True from tiramisu.setting import list_sessions, delete_session - assert 'test_persistent' in list_sessions + assert 'test_persistent' in list_sessions() delete_session('test_persistent') c = Config(o, session_id='test_persistent', persistent=True) - assert c.b is False + assert c.b is None diff --git a/tiramisu/config.py b/tiramisu/config.py index 1d7beca..a0c3982 100644 --- a/tiramisu/config.py +++ b/tiramisu/config.py @@ -20,6 +20,7 @@ # the rough pypy's guys: http://codespeak.net/svn/pypy/dist/pypy/config/ # the whole pypy projet is under MIT licence # ____________________________________________________________ +import weakref from tiramisu.error import PropertiesOptionError, ConfigError from tiramisu.option import OptionDescription, Option, SymLinkOption, \ BaseInformation @@ -47,14 +48,14 @@ class SubConfig(BaseInformation): ).format(type(descr))) self._impl_descr = descr # sub option descriptions - if not isinstance(context, SubConfig): - raise ValueError('context must be a SubConfig') + if not isinstance(context, weakref.ReferenceType): + raise ValueError('context must be a Weakref') self._impl_context = context self._impl_path = subpath def cfgimpl_reset_cache(self, only_expired=False, only=('values', 'settings')): - self.cfgimpl_get_context().cfgimpl_reset_cache(only_expired, only) + self._cfgimpl_get_context().cfgimpl_reset_cache(only_expired, only) def cfgimpl_get_home_by_path(self, path, force_permissive=False, force_properties=None): @@ -148,8 +149,8 @@ class SubConfig(BaseInformation): __repr__ = __str__ - def cfgimpl_get_context(self): - return self._impl_context + def _cfgimpl_get_context(self): + return self._impl_context() def cfgimpl_get_description(self): if self._impl_descr is None: @@ -159,10 +160,10 @@ class SubConfig(BaseInformation): return self._impl_descr def cfgimpl_get_settings(self): - return self.cfgimpl_get_context()._impl_settings + return self._cfgimpl_get_context()._impl_settings def cfgimpl_get_values(self): - return self.cfgimpl_get_context()._impl_values + return self._cfgimpl_get_context()._impl_values # ____________________________________________________________ # attribute methods @@ -186,7 +187,7 @@ class SubConfig(BaseInformation): self.cfgimpl_get_values().setitem(child, value, path, force_permissive=force_permissive) else: - context = self.cfgimpl_get_context() + context = self._cfgimpl_get_context() path = context.cfgimpl_get_description().impl_get_path_by_opt( child._opt) context._setattr(path, value, force_permissive=force_permissive) @@ -222,7 +223,7 @@ class SubConfig(BaseInformation): subpath = self._impl_path + '.' + name # symlink options if isinstance(opt_or_descr, SymLinkOption): - context = self.cfgimpl_get_context() + context = self._cfgimpl_get_context() path = context.cfgimpl_get_description().impl_get_path_by_opt( opt_or_descr._opt) return context._getattr(path, validate=validate, @@ -233,7 +234,7 @@ class SubConfig(BaseInformation): opt_or_descr, True, False, path=subpath, force_permissive=force_permissive, force_properties=force_properties) - return SubConfig(opt_or_descr, self.cfgimpl_get_context(), subpath) + return SubConfig(opt_or_descr, self._impl_context, subpath) else: return self.cfgimpl_get_values().getitem( opt_or_descr, path=subpath, @@ -250,7 +251,7 @@ class SubConfig(BaseInformation): :param byvalue: filter by the option's value :returns: list of matching Option objects """ - return self.cfgimpl_get_context()._find(bytype, byname, byvalue, + return self._cfgimpl_get_context()._find(bytype, byname, byvalue, first=False, type_=type_, _subpath=self.cfgimpl_get_path() @@ -266,7 +267,7 @@ class SubConfig(BaseInformation): :param byvalue: filter by the option's value :returns: list of matching Option objects """ - return self.cfgimpl_get_context()._find( + return self._cfgimpl_get_context()._find( bytype, byname, byvalue, first=True, type_=type_, _subpath=self.cfgimpl_get_path(), display_error=display_error) @@ -400,14 +401,14 @@ class SubConfig(BaseInformation): "option")) if withoption is not None: mypath = self.cfgimpl_get_path() - for path in self.cfgimpl_get_context()._find(bytype=Option, - byname=withoption, - byvalue=withvalue, - first=False, - type_='path', - _subpath=mypath): + for path in self._cfgimpl_get_context()._find(bytype=Option, + byname=withoption, + byvalue=withvalue, + first=False, + type_='path', + _subpath=mypath): path = '.'.join(path.split('.')[:-1]) - opt = self.cfgimpl_get_context().cfgimpl_get_description( + opt = self._cfgimpl_get_context().cfgimpl_get_description( ).impl_get_opt_by_path(path) if mypath is not None: if mypath == path: @@ -453,7 +454,7 @@ class SubConfig(BaseInformation): def cfgimpl_get_path(self): descr = self.cfgimpl_get_description() - context_descr = self.cfgimpl_get_context().cfgimpl_get_description() + context_descr = self._cfgimpl_get_context().cfgimpl_get_description() return context_descr.impl_get_path_by_opt(descr) @@ -504,7 +505,7 @@ class CommonConfig(SubConfig): # ____________________________________________________________ class Config(CommonConfig): "main configuration management entry" - __slots__ = tuple() + __slots__ = ('__weakref__', ) def __init__(self, descr, session_id=None, persistent=False): """ Configuration option management master class @@ -522,7 +523,7 @@ class Config(CommonConfig): storage = get_storage(self, session_id, persistent) self._impl_settings = Settings(self, storage) self._impl_values = Values(self, storage) - super(Config, self).__init__(descr, self) + super(Config, self).__init__(descr, weakref.ref(self)) self._impl_build_all_paths() self._impl_meta = None self._impl_informations = {} diff --git a/tiramisu/setting.py b/tiramisu/setting.py index 836278f..e2620cd 100644 --- a/tiramisu/setting.py +++ b/tiramisu/setting.py @@ -22,6 +22,7 @@ # ____________________________________________________________ from time import time from copy import copy +import weakref from tiramisu.error import (RequirementError, PropertiesOptionError, ConstError, ConfigError) from tiramisu.i18n import _ @@ -253,7 +254,7 @@ class Settings(object): """ # generic owner self._owner = owners.user - self.context = context + self.context = weakref.ref(context) import_lib = 'tiramisu.storage.{0}.setting'.format(storage.storage) self._p_ = __import__(import_lib, globals(), locals(), ['Settings'], -1).Settings(storage) @@ -287,7 +288,7 @@ class Settings(object): if opt is not None and _path is None: _path = self._get_opt_path(opt) self._p_.reset_properties(_path) - self.context.cfgimpl_reset_cache() + self.context().cfgimpl_reset_cache() def _getproperties(self, opt=None, path=None, is_apply_req=True): if opt is None: @@ -337,7 +338,7 @@ class Settings(object): self._p_.reset_properties(path) else: self._p_.setproperties(path, properties) - self.context.cfgimpl_reset_cache() + self.context().cfgimpl_reset_cache() #____________________________________________________________ def validate_properties(self, opt_or_descr, is_descr, is_write, path, @@ -377,7 +378,7 @@ class Settings(object): properties -= frozenset(('mandatory', 'frozen')) else: if 'mandatory' in properties and \ - not self.context.cfgimpl_get_values()._isempty( + not self.context().cfgimpl_get_values()._isempty( opt_or_descr, value): properties.remove('mandatory') if is_write and 'everything_frozen' in self_properties: @@ -494,7 +495,7 @@ class Settings(object): " '{0}' with requirement on: " "'{1}'").format(path, reqpath)) try: - value = self.context._getattr(reqpath, + value = self.context()._getattr(reqpath, force_permissive=True) except PropertiesOptionError, err: if not transitive: @@ -519,4 +520,4 @@ class Settings(object): return calc_properties def _get_opt_path(self, opt): - return self.context.cfgimpl_get_description().impl_get_path_by_opt(opt) + return self.context().cfgimpl_get_description().impl_get_path_by_opt(opt) diff --git a/tiramisu/storage/dictionary/storage.py b/tiramisu/storage/dictionary/storage.py index 8da0587..4006a02 100644 --- a/tiramisu/storage/dictionary/storage.py +++ b/tiramisu/storage/dictionary/storage.py @@ -52,7 +52,10 @@ class Storage(object): _list_sessions.append(self.session_id) def __del__(self): - _list_sessions.remove(self.session_id) + try: + _list_sessions.remove(self.session_id) + except AttributeError: + pass class Cache(object): diff --git a/tiramisu/value.py b/tiramisu/value.py index cd57a4c..c654206 100644 --- a/tiramisu/value.py +++ b/tiramisu/value.py @@ -19,6 +19,7 @@ # ____________________________________________________________ from time import time from copy import copy +import weakref from tiramisu.error import ConfigError, SlaveError from tiramisu.setting import owners, multitypes, expires_time from tiramisu.autolib import carry_out_calculation @@ -40,7 +41,7 @@ class Values(object): :param context: the context is the home config's values """ - self.context = context + self.context = weakref.ref(context) # the storage type is dictionary or sqlite3 import_lib = 'tiramisu.storage.{0}.value'.format(storage.storage) self._p_ = __import__(import_lib, globals(), locals(), ['Values'], @@ -52,7 +53,7 @@ class Values(object): :param opt: the `option.Option()` object """ - meta = self.context.cfgimpl_get_meta() + meta = self.context().cfgimpl_get_meta() if meta is not None: value = meta.cfgimpl_get_values()[opt] else: @@ -105,10 +106,10 @@ class Values(object): if path is None: path = self._get_opt_path(opt) if self._p_.hasvalue(path): - setting = self.context.cfgimpl_get_settings() - opt.impl_validate(opt.impl_getdefault(), self.context, + setting = self.context().cfgimpl_get_settings() + opt.impl_validate(opt.impl_getdefault(), self.context(), 'validator' in setting) - self.context.cfgimpl_reset_cache() + self.context().cfgimpl_reset_cache() if (opt.impl_is_multi() and opt.impl_get_multitype() == multitypes.master): for slave in opt.impl_get_master_slaves(): @@ -136,7 +137,7 @@ class Values(object): callback, callback_params = opt._callback if callback_params is None: callback_params = {} - return carry_out_calculation(opt._name, config=self.context, + return carry_out_calculation(opt._name, config=self.context(), callback=callback, callback_params=callback_params, index=index) @@ -160,7 +161,7 @@ class Values(object): return value val = self._getitem(opt, path, validate, force_permissive, force_properties, validate_properties) - if 'expire' in self.context.cfgimpl_get_settings() and validate and \ + if 'expire' in self.context().cfgimpl_get_settings() and validate and \ validate_properties and force_permissive is False and \ force_properties is None: if ntime is None: @@ -172,7 +173,7 @@ class Values(object): def _getitem(self, opt, path, validate, force_permissive, force_properties, validate_properties): # options with callbacks - setting = self.context.cfgimpl_get_settings() + setting = self.context().cfgimpl_get_settings() is_frozen = 'frozen' in setting[opt] # if value is callback and is not set # or frozen with force_default_on_freeze @@ -183,7 +184,7 @@ class Values(object): if (opt.impl_is_multi() and opt.impl_get_multitype() == multitypes.slave): masterp = self._get_opt_path(opt.impl_get_master_slaves()) - mastervalue = getattr(self.context, masterp) + mastervalue = getattr(self.context(), masterp) lenmaster = len(mastervalue) if lenmaster == 0: value = [] @@ -203,11 +204,11 @@ class Values(object): elif is_frozen and 'force_default_on_freeze' in setting[opt]: value = self._getdefault(opt) if opt.impl_is_multi(): - value = Multi(value, self.context, opt, path, validate) + value = Multi(value, self.context(), opt, path, validate) else: value = self._getvalue(opt, path, validate) if validate: - opt.impl_validate(value, self.context, 'validator' in setting) + opt.impl_validate(value, self.context(), 'validator' in setting) if self._is_default_owner(path) and \ 'force_store_value' in setting[opt]: self.setitem(opt, value, path, is_write=False) @@ -225,8 +226,8 @@ class Values(object): # is_write is, for example, used with "force_store_value" # user didn't change value, so not write # valid opt - opt.impl_validate(value, self.context, - 'validator' in self.context.cfgimpl_get_settings()) + opt.impl_validate(value, self.context(), + 'validator' in self.context().cfgimpl_get_settings()) if opt.impl_is_multi() and not isinstance(value, Multi): value = Multi(value, self.context, opt, path) self._setvalue(opt, path, value, force_permissive=force_permissive, @@ -235,14 +236,14 @@ class Values(object): def _setvalue(self, opt, path, value, force_permissive=False, force_properties=None, is_write=True, validate_properties=True): - self.context.cfgimpl_reset_cache() + self.context().cfgimpl_reset_cache() if validate_properties: - setting = self.context.cfgimpl_get_settings() + setting = self.context().cfgimpl_get_settings() setting.validate_properties(opt, False, is_write, value=value, path=path, force_permissive=force_permissive, force_properties=force_properties) - owner = self.context.cfgimpl_get_settings().getowner() + owner = self.context().cfgimpl_get_settings().getowner() self._p_.setvalue(path, value, owner) def getowner(self, opt): @@ -259,7 +260,7 @@ class Values(object): def _getowner(self, path): owner = self._p_.getowner(path, owners.default) - meta = self.context.cfgimpl_get_meta() + meta = self.context().cfgimpl_get_meta() if owner is owners.default and meta is not None: owner = meta.cfgimpl_get_values()._getowner(path) return owner @@ -311,7 +312,7 @@ class Values(object): :param opt: the `option.Option` object :returns: a string with points like "gc.dummy.my_option" """ - return self.context.cfgimpl_get_description().impl_get_path_by_opt(opt) + return self.context().cfgimpl_get_description().impl_get_path_by_opt(opt) # ____________________________________________________________ # multi types @@ -330,6 +331,8 @@ class Multi(list): """ self.opt = opt self.path = path + if not isinstance(context, weakref.ReferenceType): + raise ValueError('context must be a Weakref') self.context = context if not isinstance(value, list): value = [value] @@ -341,13 +344,13 @@ class Multi(list): def _valid_slave(self, value): #if slave, had values until master's one - masterp = self.context.cfgimpl_get_description().impl_get_path_by_opt( + masterp = self.context().cfgimpl_get_description().impl_get_path_by_opt( self.opt.impl_get_master_slaves()) - mastervalue = getattr(self.context, masterp) + mastervalue = getattr(self.context(), masterp) masterlen = len(mastervalue) valuelen = len(value) if valuelen > masterlen or (valuelen < masterlen and - not self.context.cfgimpl_get_values( + not self.context().cfgimpl_get_values( )._is_default_owner(self.path)): raise SlaveError(_("invalid len for the slave: {0}" " which has {1} as master").format( @@ -360,7 +363,7 @@ class Multi(list): def _valid_master(self, value): masterlen = len(value) - values = self.context.cfgimpl_get_values() + values = self.context().cfgimpl_get_values() for slave in self.opt._master_slaves: path = values._get_opt_path(slave) if not values._is_default_owner(path): @@ -379,7 +382,7 @@ class Multi(list): self._validate(value) #assume not checking mandatory property super(Multi, self).__setitem__(key, value) - self.context.cfgimpl_get_values()._setvalue(self.opt, self.path, self) + self.context().cfgimpl_get_values()._setvalue(self.opt, self.path, self) def append(self, value, force=False): """the list value can be updated (appened) @@ -390,7 +393,7 @@ class Multi(list): raise SlaveError(_("cannot append a value on a multi option {0}" " which is a slave").format(self.opt._name)) elif self.opt.impl_get_multitype() == multitypes.master: - values = self.context.cfgimpl_get_values() + values = self.context().cfgimpl_get_values() if value is None and self.opt.impl_has_callback(): value = values._getcallback_value(self.opt) #Force None il return a list @@ -398,7 +401,7 @@ class Multi(list): value = None self._validate(value) super(Multi, self).append(value) - self.context.cfgimpl_get_values()._setvalue(self.opt, self.path, self, validate_properties=not force) + self.context().cfgimpl_get_values()._setvalue(self.opt, self.path, self, validate_properties=not force) if not force and self.opt.impl_get_multitype() == multitypes.master: for slave in self.opt.impl_get_master_slaves(): path = values._get_opt_path(slave) @@ -425,7 +428,7 @@ class Multi(list): raise SlaveError(_("cannot sort multi option {0} if master or slave" "").format(self.opt._name)) super(Multi, self).sort(cmp=cmp, key=key, reverse=reverse) - self.context.cfgimpl_get_values()._setvalue(self.opt, self.path, self) + self.context().cfgimpl_get_values()._setvalue(self.opt, self.path, self) def reverse(self): if self.opt.impl_get_multitype() in [multitypes.slave, @@ -433,7 +436,7 @@ class Multi(list): raise SlaveError(_("cannot reverse multi option {0} if master or " "slave").format(self.opt._name)) super(Multi, self).reverse() - self.context.cfgimpl_get_values()._setvalue(self.opt, self.path, self) + self.context().cfgimpl_get_values()._setvalue(self.opt, self.path, self) def insert(self, index, obj): if self.opt.impl_get_multitype() in [multitypes.slave, @@ -441,7 +444,7 @@ class Multi(list): raise SlaveError(_("cannot insert multi option {0} if master or " "slave").format(self.opt._name)) super(Multi, self).insert(index, obj) - self.context.cfgimpl_get_values()._setvalue(self.opt, self.path, self) + self.context().cfgimpl_get_values()._setvalue(self.opt, self.path, self) def extend(self, iterable): if self.opt.impl_get_multitype() in [multitypes.slave, @@ -449,7 +452,7 @@ class Multi(list): raise SlaveError(_("cannot extend multi option {0} if master or " "slave").format(self.opt._name)) super(Multi, self).extend(iterable) - self.context.cfgimpl_get_values()._setvalue(self.opt, self.path, self) + self.context().cfgimpl_get_values()._setvalue(self.opt, self.path, self) def _validate(self, value): if value is not None: @@ -474,7 +477,7 @@ class Multi(list): " which is a slave").format(self.opt._name)) elif self.opt.impl_get_multitype() == multitypes.master: for slave in self.opt.impl_get_master_slaves(): - values = self.context.cfgimpl_get_values() + values = self.context().cfgimpl_get_values() if not values.is_default_owner(slave): #get multi without valid properties values.getitem(slave, @@ -482,5 +485,5 @@ class Multi(list): ).pop(key, force=True) #set value without valid properties ret = super(Multi, self).pop(key) - self.context.cfgimpl_get_values()._setvalue(self.opt, self.path, self, validate_properties=not force) + self.context().cfgimpl_get_values()._setvalue(self.opt, self.path, self, validate_properties=not force) return ret