[2051] Make sure results aren't deleted too soon

In the python part, we play a little with the reference counts. The data
source client wrapper can now hold a life keeper in addition to the
container, as some data source clients don't come from the container or
are not known.

src/lib/python/isc/datasrc/client_python.cc

@@ -28,6 +28,7 @@
 #include <datasrc/data_source.h>
 #include <datasrc/sqlite3_accessor.h>
 #include <datasrc/iterator.h>
+#include <datasrc/client_list.h>
 
 #include <dns/python/name_python.h>
 #include <dns/python/rrset_python.h>
@@ -53,10 +54,15 @@ class s_DataSourceClient : public PyObject {
 public:
     s_DataSourceClient() :
         cppobj(NULL),
-        client(NULL)
+        client(NULL),
+        keeper(NULL)
     {};
     DataSourceClientContainer* cppobj;
     DataSourceClient* client;
+    // We can't rely on the constructor or destructor being
+    // called, so this is a pointer to shared pointer, so we
+    // can call the new and delete explicitly.
+    boost::shared_ptr<ClientList::FindResult::LifeKeeper>* keeper;
 };
 
 PyObject*
@@ -250,6 +256,7 @@ DataSourceClient_init(PyObject* po_self, PyObject* args, PyObject*) {
             self->cppobj = new DataSourceClientContainer(ds_type_str,
                                                          ds_config);
             self->client = &self->cppobj->getInstance();
+            self->keeper = NULL;
             return (0);
         } else {
             return (-1);
@@ -285,8 +292,10 @@ void
 DataSourceClient_destroy(PyObject* po_self) {
     s_DataSourceClient* const self = static_cast<s_DataSourceClient*>(po_self);
     delete self->cppobj;
+    delete self->keeper;
     self->cppobj = NULL;
     self->client = NULL;
+    self->keeper = NULL;
     Py_TYPE(self)->tp_free(self);
 }
 
@@ -349,14 +358,20 @@ PyTypeObject datasourceclient_type = {
 };
 
 PyObject*
-wrapDataSourceClient(DataSourceClient* client) {
-    // There aro no exceptions here, so this is safe
+wrapDataSourceClient(DataSourceClient* client,
+                     const boost::shared_ptr<ClientList::FindResult::
+                     LifeKeeper>& life_keeper)
+{
     s_DataSourceClient *result =
         static_cast<s_DataSourceClient*>(PyObject_New(s_DataSourceClient,
                                                       &datasourceclient_type));
+    CPPPyObjectContainer<s_DataSourceClient, DataSourceClientContainer>
+        container(result);
     result->cppobj = NULL;
+    result->keeper =
+        new boost::shared_ptr<ClientList::FindResult::LifeKeeper>(life_keeper);
     result->client = client;
-    return (result);
+    return (container.release());
 }
 
 } // namespace python

src/lib/python/isc/datasrc/client_python.h

@@ -15,6 +15,8 @@
 #ifndef __PYTHON_DATASRC_CLIENT_H
 #define __PYTHON_DATASRC_CLIENT_H 1
 
+#include <datasrc/client_list.h>
+
 #include <Python.h>
 
 namespace isc {
@@ -26,7 +28,11 @@ namespace python {
 extern PyTypeObject datasourceclient_type;
 
 // TODO: Documentation, warning
-PyObject* wrapDataSourceClient(DataSourceClient* client);
+PyObject*
+wrapDataSourceClient(DataSourceClient* client,
+                     const boost::shared_ptr<ClientList::FindResult::
+                     LifeKeeper>& life_keeper = boost::shared_ptr<ClientList::
+                     FindResult::LifeKeeper>());
 
 } // namespace python
 } // namespace datasrc

src/lib/python/isc/datasrc/configurableclientlist_python.cc

@@ -132,13 +132,22 @@ ConfigurableClientList_find(PyObject* po_self, PyObject* args) {
                 // reference counts correctly.
                 dsrc.reset(Py_BuildValue(""));
             } else {
-                dsrc.reset(wrapDataSourceClient(result.dsrc_client_));
+                // Make sure we have a keeper there too, so it doesn't
+                // die when the underlying client list dies or is
+                // reconfigured.
+                //
+                // However, as it is inside the C++ part, is there a
+                // reasonable way to test it?
+                dsrc.reset(wrapDataSourceClient(result.dsrc_client_,
+                                                result.life_keeper_));
             }
             PyObjectContainer finder;
             if (result.finder_ == NULL) {
                 finder.reset(Py_BuildValue(""));
             } else {
-                finder.reset(createZoneFinderObject(result.finder_));
+                // Make sure it keeps the data source client alive.
+                finder.reset(createZoneFinderObject(result.finder_,
+                                                    dsrc.get()));
             }
             PyObjectContainer exact(PyBool_FromLong(result.exact_match_));
 

src/lib/python/isc/datasrc/tests/clientlist_test.py

@@ -18,6 +18,7 @@ import isc.datasrc
 import isc.dns
 import unittest
 import os
+import sys
 
 TESTDATA_PATH = os.environ['GLOBAL_TESTDATA_PATH'] + os.sep
 
@@ -103,6 +104,12 @@ class ClientListTest(unittest.TestCase):
         self.assertTrue(isinstance(dsrc, isc.datasrc.DataSourceClient))
         self.assertIsNotNone(finder)
         self.assertTrue(isinstance(finder, isc.datasrc.ZoneFinder))
+        # Check the finder holds a reference to the data source
+        # Note that one reference is kept in the parameter list
+        # of getrefcount
+        self.assertEqual(3, sys.getrefcount(dsrc))
+        finder = None
+        self.assertEqual(2, sys.getrefcount(dsrc))
         # We check an exact match in test_configure already
         self.assertFalse(exact)
         dsrc, finder, exact = clist.find(isc.dns.Name("sub.example.org"),