Commits

Mike Bayer committed dee2bc0

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]

  • Participants
  • Parent commits d39d27e

Comments (0)

Files changed (7)

doc/build/changelog/changelog_08.rst

     :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
 

lib/sqlalchemy/orm/instrumentation.py

         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

lib/sqlalchemy/orm/mapper.py

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

lib/sqlalchemy/orm/session.py

 
     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."
 
         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())
 

lib/sqlalchemy/orm/state.py

         return bool(self.key)
 
     def _detach(self):
-        self.session_id = None
+        self.session_id = self._strong_obj = None
 
     def _dispose(self):
         self._detach()
             instance_dict.discard(self)
 
         self.callables = {}
-        self.session_id = None
+        self.session_id = self._strong_obj = None
         del self.obj
 
     def obj(self):
         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',
             modified_set.discard(self)
 
         self.modified = False
+        self._strong_obj = None
 
         self.committed_state.clear()
 
         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:
 
             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 "
         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)
 

test/orm/test_instrumentation.py

         # 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."""

test/orm/test_session.py

             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