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]

4 files changed, 28 insertions, 17 deletions

diff --git a/lib/sqlalchemy/orm/instrumentation.py b/lib/sqlalchemy/orm/instrumentation.py
index 51cf9edeb..0e71494c4 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 914c29b7f..c08d91b57 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 408b119a0..361ab65e6 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 4bc689e94..193678c2f 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)