From: Mike Bayer Date: Sat, 5 Mar 2011 01:52:22 +0000 (-0500) Subject: - some changes to the identity map regarding X-Git-Tag: rel_0_7b3~43 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=403d78697003805879be2fbad4693830e6d8d4c6;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git - some changes to the identity map regarding rare weakref callbacks during iterations. The mutex has been removed as it apparently can cause a reentrant (i.e. in one thread) deadlock, perhaps when gc collects objects at the point of iteration in order to gain more memory. It is hoped that "dictionary changed during iteration" will be exceedingly rare as iteration methods internally acquire the full list of objects in a single values() call. [ticket:2087] --- diff --git a/CHANGES b/CHANGES index b6b6b3bf8e..202e34434e 100644 --- a/CHANGES +++ b/CHANGES @@ -5,6 +5,18 @@ CHANGES ======= 0.7.0b3 ======= +- orm + - some changes to the identity map regarding + rare weakref callbacks during iterations. + The mutex has been removed as it apparently + can cause a reentrant (i.e. in one thread) deadlock, + perhaps when gc collects objects at the point of + iteration in order to gain more memory. It is hoped + that "dictionary changed during iteration" will + be exceedingly rare as iteration methods internally + acquire the full list of objects in a single values() + call. [ticket:2087] + - sql - Added a fully descriptive error message for the case where Column is subclassed and _make_proxy() diff --git a/lib/sqlalchemy/orm/identity.py b/lib/sqlalchemy/orm/identity.py index b3a7f8bc3f..a53d9f52db 100644 --- a/lib/sqlalchemy/orm/identity.py +++ b/lib/sqlalchemy/orm/identity.py @@ -5,8 +5,6 @@ # the MIT License: http://www.opensource.org/licenses/mit-license.php import weakref - -from sqlalchemy import util as base_util from sqlalchemy.orm import attributes @@ -22,9 +20,6 @@ class IdentityMap(dict): def add(self, state): raise NotImplementedError() - def remove(self, state): - raise NotImplementedError() - def update(self, dict): raise NotImplementedError("IdentityMap uses add() to insert data") @@ -83,7 +78,6 @@ class IdentityMap(dict): class WeakInstanceDict(IdentityMap): def __init__(self): IdentityMap.__init__(self) - self._remove_mutex = base_util.threading.Lock() def __getitem__(self, key): state = dict.__getitem__(self, key) @@ -143,27 +137,6 @@ class WeakInstanceDict(IdentityMap): dict.__setitem__(self, key, state) self._manage_incoming_state(state) - def remove_key(self, key): - state = dict.__getitem__(self, key) - self.remove(state) - - def remove(self, state): - self._remove_mutex.acquire() - try: - if dict.pop(self, state.key) is not state: - raise AssertionError( - "State %s is not present in this " - "identity map" % state) - finally: - self._remove_mutex.release() - - self._manage_removed_state(state) - - def discard(self, state): - if self.contains_state(state): - dict.__delitem__(self, state.key) - self._manage_removed_state(state) - def get(self, key, default=None): if not dict.__contains__(self, key): return default @@ -175,53 +148,53 @@ class WeakInstanceDict(IdentityMap): return default return o - def items(self): + def _items(self): + values = self.all_states() + result = [] + for state in values: + value = state.obj() + if value is not None: + result.append((state.key, value)) + return result + + def _values(self): + values = self.all_states() + result = [] + for state in values: + value = state.obj() + if value is not None: + result.append(value) + + return result + + # Py3K + #def items(self): + # return iter(self._items()) + # + #def values(self): + # return iter(self._values()) # Py2K - return list(self.iteritems()) - + items = _items def iteritems(self): - # end Py2K - self._remove_mutex.acquire() - try: - result = [] - for state in dict.values(self): - value = state.obj() - if value is not None: - result.append((state.key, value)) - - return iter(result) - finally: - self._remove_mutex.release() - - def values(self): - # Py2K - return list(self.itervalues()) + return iter(self.items()) + values = _values def itervalues(self): + return iter(self.values()) # end Py2K - self._remove_mutex.acquire() - try: - result = [] - for state in dict.values(self): - value = state.obj() - if value is not None: - result.append(value) - - return iter(result) - finally: - self._remove_mutex.release() def all_states(self): - self._remove_mutex.acquire() - try: - # Py3K - # return list(dict.values(self)) + # Py3K + # return list(dict.values(self)) + # Py2K + return dict.values(self) + # end Py2K - # Py2K - return dict.values(self) - # end Py2K - finally: - self._remove_mutex.release() + def discard(self, state): + st = dict.get(self, state.key, None) + if st is state: + dict.__delitem__(self, state.key) + self._manage_removed_state(state) def prune(self): return 0 @@ -256,21 +229,13 @@ class StrongInstanceDict(IdentityMap): dict.__setitem__(self, state.key, state.obj()) self._manage_incoming_state(state) - def remove(self, state): - if attributes.instance_state(dict.pop(self, state.key)) \ - is not state: - raise AssertionError('State %s is not present in this ' - 'identity map' % state) - self._manage_removed_state(state) - def discard(self, state): - if self.contains_state(state): - dict.__delitem__(self, state.key) - self._manage_removed_state(state) - - def remove_key(self, key): - state = attributes.instance_state(dict.__getitem__(self, key)) - self.remove(state) + obj = dict.get(self, state.key, None) + if obj is not None: + st = attributes.instance_state(obj) + if st is state: + dict.__delitem__(self, state.key) + self._manage_removed_state(state) def prune(self): """prune unreferenced, non-dirty states.""" diff --git a/lib/sqlalchemy/orm/state.py b/lib/sqlalchemy/orm/state.py index e5e74eafb4..e29c5c03ad 100644 --- a/lib/sqlalchemy/orm/state.py +++ b/lib/sqlalchemy/orm/state.py @@ -68,10 +68,7 @@ class InstanceState(object): def _cleanup(self, ref): instance_dict = self._instance_dict() if instance_dict: - try: - instance_dict.remove(self) - except AssertionError: - pass + instance_dict.discard(self) self.callables = {} self.session_id = None @@ -516,10 +513,7 @@ class MutableAttrInstanceState(InstanceState): else: instance_dict = self._instance_dict() if instance_dict: - try: - instance_dict.remove(self) - except AssertionError: - pass + instance_dict.discard(self) self.dispose() def __resurrect(self): diff --git a/test/orm/test_session.py b/test/orm/test_session.py index f0a9255410..9ef00e6472 100644 --- a/test/orm/test_session.py +++ b/test/orm/test_session.py @@ -1280,7 +1280,7 @@ class DisposedStates(_base.MappedTest): sess.identity_map.all_states = lambda : all_states for obj in objs: state = attributes.instance_state(obj) - sess.identity_map.remove(state) + sess.identity_map.discard(state) state.dispose() def _test_session(self, **kwargs):