Merge pull request #2015 from KLayout/bugfix/issue-2012

Bugfix/issue 2012

Author: klayoutmatthias

src/pya/pya/pyaConvert.cc

@@ -440,7 +440,7 @@ object_to_python (void *obj, PYAObjectBase *self, const gsi::ClassBase *cls, boo
       obj = clsact->create_from_adapted (obj);
     }
 
-    //  we wil own the new object
+    //  we will own the new object
     pass_obj = true;
 
   }
@@ -488,6 +488,7 @@ object_to_python (void *obj, PYAObjectBase *self, const gsi::ClassBase *cls, boo
     PYAObjectBase *new_object = PYAObjectBase::from_pyobject_unsafe (new_pyobject);
     new (new_object) PYAObjectBase (clsact, new_pyobject);
     new_object->set (obj, pass_obj, is_const, can_destroy);
+
     return new_pyobject;
 
   }

src/pya/pya/pyaObject.cc

@@ -238,9 +238,16 @@ PYAObjectBase::object_destroyed ()
 
     detach ();
 
-    //  NOTE: this may delete "this"!
-    if (!prev_owner) {
-      Py_DECREF (py_object ());
+    if (! prev_owner) {
+      const gsi::ClassBase *cls = cls_decl ();
+      if (cls && cls->is_managed ()) {
+        //  If the object was owned on C++ side before, we need to decrement the
+        //  reference count to reflect the fact, that there no longer is an external
+        //  owner.
+        //  NOTE: this may delete "this", hence we return
+        Py_DECREF (py_object ());
+        return;
+      }
     }
 
   }
@@ -249,31 +256,44 @@ PYAObjectBase::object_destroyed ()
 void 
 PYAObjectBase::release ()
 {
+  //  "release" means to release ownership of the C++ object on the C++ side.
+  //  In other words: to transfer ownership to the script side. Specifically to
+  //  transfer it to the Python domain.
+
   //  If the object is managed we first reset the ownership of all other clients
   //  and then make us the owner
   const gsi::ClassBase *cls = cls_decl ();
   if (cls && cls->is_managed ()) {
     void *o = obj ();
     if (o) {
+      //  NOTE: "keep" means "move ownership of the C++ object to C++". In other words,
+      //  release ownership of the C++ object on script side.
       cls->gsi_object (o)->keep ();
+      if (! m_owned) {
+        //  We have to *decrement* the reference count as now there is no other entity
+        //  holding a reference to this Python object.
+        //  NOTE: this may delete "this", hence we return
+        m_owned = true;
+        Py_DECREF (py_object ());
+        return;
+      }
     }
   }
 
-  //  NOTE: this is fairly dangerous
-  if (!m_owned) {
-    m_owned = true;
-    //  NOTE: this may delete "this"! TODO: this should not happen. Can we assert that somehow?
-    Py_DECREF (py_object ());
-  }
+  m_owned = true;
 }
 
 void
 PYAObjectBase::keep_internal ()
 {
   if (m_owned) {
+    //  "keep" means to transfer ownership of the C++ object to C++ side, while
+    //  "m_owned" refers to ownership on the Python side. So if we perform this
+    //  transfer, we need to reflect the fact that there is another entity holding
+    //  a reference.
     Py_INCREF (py_object ());
-    m_owned = false;
   }
+  m_owned = false;
 }
 
 void 
@@ -284,9 +304,11 @@ PYAObjectBase::keep ()
     void *o = obj ();
     if (o) {
       if (cls->is_managed ()) {
+        //  dispatch the keep notification - this will call "keep_internal" through the
+        //  event handler (StatusChangedListener)
         cls->gsi_object (o)->keep ();
       } else {
-        keep_internal ();
+        m_owned = false;
       }
     }
   }
@@ -341,16 +363,18 @@ PYAObjectBase::set (void *obj, bool owned, bool const_ref, bool can_destroy)
 
   if (cls->is_managed ()) {
     gsi::ObjectBase *gsi_object = cls->gsi_object (m_obj);
-    //  Consider the case of "keep inside constructor"
     if (gsi_object->already_kept ()) {
-      keep_internal ();
+      //  Consider the case of "keep inside constructor"
+      m_owned = false;
+    }
+    if (! m_owned) {
+      //  "m_owned = false" means ownership of the C++ object is on C++ side,
+      //  and not on script side. In that case, we need to increment the
+      //  reference count to reflect the fact that there is an external owner.
+      Py_INCREF (py_object ());
     }
     gsi_object->status_changed_event ().add (mp_listener, &StatusChangedListener::object_status_changed);
   }
-
-  if (!m_owned) {
-    Py_INCREF (py_object ());
-  }
 }
 
 //  TODO: a static (singleton) instance is not thread-safe
@@ -587,7 +611,7 @@ PYAObjectBase::obj ()
       throw tl::Exception (tl::to_string (tr ("Object has been destroyed already")));
     } else {
       //  delayed creation of a detached C++ object ..
-      set(cls_decl ()->create (), true, false, true);
+      set (cls_decl ()->create (), true, false, true);
     }
   }
 

testdata/python/dbLayoutTest.py

@@ -684,7 +684,7 @@ def test_6_Layout_props2(self):
     ly = pya.Layout(True)
     pv = [ [ 17, "a" ], [ "b", [ 1, 5, 7 ] ] ]
     pid = ly.properties_id( pv )
-    # does not work? @@@
+    # does not work? 
     # pv = { 17: "a", "b": [ 1, 5, 7 ] }
     # pid2 = ly.properties_id( pv )
     # self.assertEqual( pid, pid2 )

testdata/python/dbShapesTest.py

@@ -24,7 +24,7 @@
 class DBShapesTest(unittest.TestCase):
 
   # Shape objects as hashes
-  def test_12(self):
+  def test_1(self):
 
     s = pya.Shapes()
     s1 = s.insert(pya.Box(1, 2, 3, 4))
@@ -50,6 +50,18 @@ def test_12(self):
     self.assertEqual(h[s2], 2)
     self.assertEqual(h[s3], 3)
 
+  # Issue #2012 (reference count)
+  def test_2(self):
+
+    ly = pya.Layout()
+    top = ly.create_cell("TOP")
+    l1 = ly.layer(1, 0)
+    top.shapes(l1).insert(pya.Box(0, 0, 100, 200))
+
+    shapes = top.shapes(l1)
+    self.assertEqual(sys.getrefcount(shapes), 2)
+
+
 # run unit tests
 if __name__ == '__main__':
   suite = unittest.TestLoader().loadTestsFromTestCase(DBShapesTest)