From: Mike Bayer <mike_mp@zzzcomputing.com>
Date: Sat, 20 Apr 2013 06:45:08 +0000 (-0400)
Subject: Improved the behavior of instance management regarding
X-Git-Tag: rel_0_8_1~13^2~2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=6c1972e4ecd7f2d738aa1578bc95d4a77820278d;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git

Improved the behavior of instance management regarding
the creation of strong references within the Session;
an object will no longer have an internal reference cycle
created if it's in the transient state or moves into the
detached state - the strong ref is created only when the
object is attached to a Session and is removed when the
object is detached.  This makes it somewhat safer for an
object to have a `__del__()` method, even though this is
not recommended, as relationships with backrefs produce
cycles too.  A warning has been added when a class with
a `__del__()` method is mapped.
[ticket:2708]
---

diff --git a/doc/build/changelog/changelog_08.rst b/doc/build/changelog/changelog_08.rst
index c443e2735d..bdba906bb5 100644
--- a/doc/build/changelog/changelog_08.rst
+++ b/doc/build/changelog/changelog_08.rst
@@ -6,6 +6,22 @@
 .. changelog::
     :version: 0.8.1
 
+    .. change::
+      :tags: bug
+      :tickets: 2708
+
+      Improved the behavior of instance management regarding
+      the creation of strong references within the Session;
+      an object will no longer have an internal reference cycle
+      created if it's in the transient state or moves into the
+      detached state - the strong ref is created only when the
+      object is attached to a Session and is removed when the
+      object is detached.  This makes it somewhat safer for an
+      object to have a `__del__()` method, even though this is
+      not recommended, as relationships with backrefs produce
+      cycles too.  A warning has been added when a class with
+      a `__del__()` method is mapped.
+
     .. change::
       :tags: bug, mysql
       :pullreq: 54
diff --git a/lib/sqlalchemy/orm/instrumentation.py b/lib/sqlalchemy/orm/instrumentation.py
index 51cf9edeb8..0e71494c44 100644
--- a/lib/sqlalchemy/orm/instrumentation.py
+++ b/lib/sqlalchemy/orm/instrumentation.py
@@ -72,6 +72,13 @@ class ClassManager(dict):
         self.manage()
         self._instrument_init()
 
+        if '__del__' in class_.__dict__:
+            util.warn("__del__() method on class %s will "
+                        "cause unreachable cycles and memory leaks, "
+                        "as SQLAlchemy instrumentation often creates "
+                        "reference cycles.  Please remove this method." %
+                        class_)
+
     dispatch = event.dispatcher(events.InstanceEvents)
 
     @property
diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py
index 914c29b7fa..c08d91b570 100644
--- a/lib/sqlalchemy/orm/mapper.py
+++ b/lib/sqlalchemy/orm/mapper.py
@@ -761,8 +761,9 @@ class Mapper(_InspectionAttr):
             del self._configure_failed
 
         if not self.non_primary and \
+            self.class_manager is not None and \
             self.class_manager.is_mapped and \
-            self.class_manager.mapper is self:
+                self.class_manager.mapper is self:
             instrumentation.unregister_class(self.class_)
 
     def _configure_pks(self):
diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py
index 408b119a07..361ab65e6e 100644
--- a/lib/sqlalchemy/orm/session.py
+++ b/lib/sqlalchemy/orm/session.py
@@ -1727,13 +1727,13 @@ class Session(_SessionClassMethods):
 
     def _before_attach(self, state):
         if state.session_id != self.hash_key and \
-            self.dispatch.before_attach:
+                self.dispatch.before_attach:
             self.dispatch.before_attach(self, state.obj())
 
     def _attach(self, state, include_before=False):
         if state.key and \
             state.key in self.identity_map and \
-            not self.identity_map.contains_state(state):
+                not self.identity_map.contains_state(state):
             raise sa_exc.InvalidRequestError("Can't attach instance "
                     "%s; another instance with key %s is already "
                     "present in this session."
@@ -1749,9 +1749,11 @@ class Session(_SessionClassMethods):
 
         if state.session_id != self.hash_key:
             if include_before and \
-                self.dispatch.before_attach:
+                    self.dispatch.before_attach:
                 self.dispatch.before_attach(self, state.obj())
             state.session_id = self.hash_key
+            if state.modified and not state._strong_obj:
+                state._strong_obj = state.obj()
             if self.dispatch.after_attach:
                 self.dispatch.after_attach(self, state.obj())
 
diff --git a/lib/sqlalchemy/orm/state.py b/lib/sqlalchemy/orm/state.py
index 4bc689e94d..193678c2ff 100644
--- a/lib/sqlalchemy/orm/state.py
+++ b/lib/sqlalchemy/orm/state.py
@@ -164,7 +164,7 @@ class InstanceState(interfaces._InspectionAttr):
         return bool(self.key)
 
     def _detach(self):
-        self.session_id = None
+        self.session_id = self._strong_obj = None
 
     def _dispose(self):
         self._detach()
@@ -176,7 +176,7 @@ class InstanceState(interfaces._InspectionAttr):
             instance_dict.discard(self)
 
         self.callables = {}
-        self.session_id = None
+        self.session_id = self._strong_obj = None
         del self.obj
 
     def obj(self):
@@ -259,9 +259,6 @@ class InstanceState(interfaces._InspectionAttr):
         self.expired = state.get('expired', False)
         self.callables = state.get('callables', {})
 
-        if self.modified:
-            self._strong_obj = inst
-
         self.__dict__.update([
             (k, state[k]) for k in (
                 'key', 'load_options',
@@ -322,6 +319,7 @@ class InstanceState(interfaces._InspectionAttr):
             modified_set.discard(self)
 
         self.modified = False
+        self._strong_obj = None
 
         self.committed_state.clear()
 
@@ -335,7 +333,7 @@ class InstanceState(interfaces._InspectionAttr):
         for key in self.manager:
             impl = self.manager[key].impl
             if impl.accepts_scalar_loader and \
-                (impl.expire_missing or key in dict_):
+                    (impl.expire_missing or key in dict_):
                 self.callables[key] = self
             old = dict_.pop(key, None)
             if impl.collection and old is not None:
@@ -435,18 +433,22 @@ class InstanceState(interfaces._InspectionAttr):
 
             self.committed_state[attr.key] = previous
 
-        # the "or not self.modified" is defensive at
-        # this point.  The assertion below is expected
-        # to be True:
         # assert self._strong_obj is None or self.modified
 
-        if self._strong_obj is None or not self.modified:
+        if (self.session_id and self._strong_obj is None) \
+                or not self.modified:
             instance_dict = self._instance_dict()
             if instance_dict:
                 instance_dict._modified.add(self)
 
-            self._strong_obj = self.obj()
-            if self._strong_obj is None:
+            # only create _strong_obj link if attached
+            # to a session
+
+            inst = self.obj()
+            if self.session_id:
+                self._strong_obj = inst
+
+            if inst is None:
                 raise orm_exc.ObjectDereferencedError(
                         "Can't emit change event for attribute '%s' - "
                         "parent object of type %s has been garbage "
@@ -467,7 +469,6 @@ class InstanceState(interfaces._InspectionAttr):
         this step if a value was not populated in state.dict.
 
         """
-        class_manager = self.manager
         for key in keys:
             self.committed_state.pop(key, None)
 
diff --git a/test/orm/test_instrumentation.py b/test/orm/test_instrumentation.py
index 3b548f0cd1..3f8fc67b68 100644
--- a/test/orm/test_instrumentation.py
+++ b/test/orm/test_instrumentation.py
@@ -445,6 +445,20 @@ class MapperInitTest(fixtures.ORMTest):
         # C is not mapped in the current implementation
         assert_raises(sa.orm.exc.UnmappedClassError, class_mapper, C)
 
+    def test_del_warning(self):
+        class A(object):
+            def __del__(self):
+                pass
+
+        assert_raises_message(
+            sa.exc.SAWarning,
+            r"__del__\(\) method on class "
+            "<class 'test.orm.test_instrumentation.A'> will cause "
+            "unreachable cycles and memory leaks, as SQLAlchemy "
+            "instrumentation often creates reference cycles.  "
+            "Please remove this method.",
+            mapper, A, self.fixture()
+        )
 
 class OnLoadTest(fixtures.ORMTest):
     """Check that Events.load is not hit in regular attributes operations."""
diff --git a/test/orm/test_session.py b/test/orm/test_session.py
index 5c89688427..7c2e8a3b86 100644
--- a/test/orm/test_session.py
+++ b/test/orm/test_session.py
@@ -857,6 +857,150 @@ class SessionStateWFixtureTest(_fixtures.FixtureTest):
             assert sa.orm.attributes.instance_state(a).session_id is None
 
 
+class NoCyclesOnTransientDetachedTest(_fixtures.FixtureTest):
+    """Test the instance_state._strong_obj link that it
+    is present only on persistent/pending objects and never
+    transient/detached.
+
+    """
+    run_inserts = None
+
+    def setup(self):
+        mapper(self.classes.User, self.tables.users)
+
+    def _assert_modified(self, u1):
+        assert sa.orm.attributes.instance_state(u1).modified
+
+    def _assert_not_modified(self, u1):
+        assert not sa.orm.attributes.instance_state(u1).modified
+
+    def _assert_cycle(self, u1):
+        assert sa.orm.attributes.instance_state(u1)._strong_obj is not None
+
+    def _assert_no_cycle(self, u1):
+        assert sa.orm.attributes.instance_state(u1)._strong_obj is None
+
+    def _persistent_fixture(self):
+        User = self.classes.User
+        u1 = User()
+        u1.name = "ed"
+        sess = Session()
+        sess.add(u1)
+        sess.flush()
+        return sess, u1
+
+    def test_transient(self):
+        User = self.classes.User
+        u1 = User()
+        u1.name = 'ed'
+        self._assert_no_cycle(u1)
+        self._assert_modified(u1)
+
+    def test_transient_to_pending(self):
+        User = self.classes.User
+        u1 = User()
+        u1.name = 'ed'
+        self._assert_modified(u1)
+        self._assert_no_cycle(u1)
+        sess = Session()
+        sess.add(u1)
+        self._assert_cycle(u1)
+        sess.flush()
+        self._assert_no_cycle(u1)
+        self._assert_not_modified(u1)
+
+    def test_dirty_persistent_to_detached_via_expunge(self):
+        sess, u1 = self._persistent_fixture()
+        u1.name = 'edchanged'
+        self._assert_cycle(u1)
+        sess.expunge(u1)
+        self._assert_no_cycle(u1)
+
+    def test_dirty_persistent_to_detached_via_close(self):
+        sess, u1 = self._persistent_fixture()
+        u1.name = 'edchanged'
+        self._assert_cycle(u1)
+        sess.close()
+        self._assert_no_cycle(u1)
+
+    def test_clean_persistent_to_detached_via_close(self):
+        sess, u1 = self._persistent_fixture()
+        self._assert_no_cycle(u1)
+        self._assert_not_modified(u1)
+        sess.close()
+        u1.name = 'edchanged'
+        self._assert_modified(u1)
+        self._assert_no_cycle(u1)
+
+    def test_detached_to_dirty_deleted(self):
+        sess, u1 = self._persistent_fixture()
+        sess.expunge(u1)
+        u1.name = 'edchanged'
+        self._assert_no_cycle(u1)
+        sess.delete(u1)
+        self._assert_cycle(u1)
+
+    def test_detached_to_dirty_persistent(self):
+        sess, u1 = self._persistent_fixture()
+        sess.expunge(u1)
+        u1.name = 'edchanged'
+        self._assert_modified(u1)
+        self._assert_no_cycle(u1)
+        sess.add(u1)
+        self._assert_cycle(u1)
+        self._assert_modified(u1)
+
+    def test_detached_to_clean_persistent(self):
+        sess, u1 = self._persistent_fixture()
+        sess.expunge(u1)
+        self._assert_no_cycle(u1)
+        self._assert_not_modified(u1)
+        sess.add(u1)
+        self._assert_no_cycle(u1)
+        self._assert_not_modified(u1)
+
+    def test_move_persistent_clean(self):
+        sess, u1 = self._persistent_fixture()
+        sess.close()
+        s2 = Session()
+        s2.add(u1)
+        self._assert_no_cycle(u1)
+        self._assert_not_modified(u1)
+
+    def test_move_persistent_dirty(self):
+        sess, u1 = self._persistent_fixture()
+        u1.name = 'edchanged'
+        self._assert_cycle(u1)
+        self._assert_modified(u1)
+        sess.close()
+        self._assert_no_cycle(u1)
+        s2 = Session()
+        s2.add(u1)
+        self._assert_cycle(u1)
+        self._assert_modified(u1)
+
+    @testing.requires.predictable_gc
+    def test_move_gc_session_persistent_dirty(self):
+        sess, u1 = self._persistent_fixture()
+        u1.name = 'edchanged'
+        self._assert_cycle(u1)
+        self._assert_modified(u1)
+        del sess
+        gc_collect()
+        self._assert_cycle(u1)
+        s2 = Session()
+        s2.add(u1)
+        self._assert_cycle(u1)
+        self._assert_modified(u1)
+
+    def test_persistent_dirty_to_expired(self):
+        sess, u1 = self._persistent_fixture()
+        u1.name = 'edchanged'
+        self._assert_cycle(u1)
+        self._assert_modified(u1)
+        sess.expire(u1)
+        self._assert_no_cycle(u1)
+        self._assert_not_modified(u1)
 
 class WeakIdentityMapTest(_fixtures.FixtureTest):
     run_inserts = None