DISPATCH-1962 Fix leak of qdr_core_subscribe

jiridanek · jiridanek · commit 071c655fd314 · 2023-12-02T21:51:58.000+01:00
undo the python undo fixup dbgsym? fixup dbgsym? fixup dbgsym? fixup dbgsym? fixup dbgsym? fixup dbgsym? fixup dbgsym? fixup dbgsym fixup dbgpkg Revert "DISPATCH-1942 Use findPython when on CMake3.15+" This reverts commit f486435 fixup too many decrefs fixup travis yml; this is the best way ;DDDD fixup travis yml? fixup try -fno-omit-frame-pointer fixup try -fno-omit-frame-pointer fixup try -fno-omit-frame-pointer fixup try -fno-omit-frame-pointer fixup try -fno-omit-frame-pointer try -fno-omit-frame-pointer WIP: attempt at improving situation around DISPATCH-1962 WIP: attempt at improving situation around DISPATCH-1962 undo the python undo fixup dbgsym? fixup dbgsym? fixup dbgsym? fixup dbgsym? fixup dbgsym? fixup dbgsym? fixup dbgsym? fixup dbgsym fixup dbgpkg Revert "DISPATCH-1942 Use findPython when on CMake3.15+" This reverts commit f486435 fixup too many decrefs fixup travis yml; this is the best way ;DDDD fixup travis yml? fixup try -fno-omit-frame-pointer fixup try -fno-omit-frame-pointer fixup try -fno-omit-frame-pointer fixup try -fno-omit-frame-pointer fixup try -fno-omit-frame-pointer try -fno-omit-frame-pointer WIP: attempt at improving situation around DISPATCH-1962 WIP: attempt at improving situation around DISPATCH-1962 undo the python undo fixup dbgsym? fixup dbgsym? fixup dbgsym? fixup dbgsym? fixup dbgsym? fixup dbgsym? fixup dbgsym? fixup dbgsym fixup dbgpkg Revert "DISPATCH-1942 Use findPython when on CMake3.15+" This reverts commit f486435 fixup too many decrefs fixup travis yml; this is the best way ;DDDD fixup travis yml? fixup try -fno-omit-frame-pointer fixup try -fno-omit-frame-pointer fixup try -fno-omit-frame-pointer fixup try -fno-omit-frame-pointer fixup try -fno-omit-frame-pointer try -fno-omit-frame-pointer WIP: attempt at improving situation around DISPATCH-1962 WIP: attempt at improving situation around DISPATCH-1962
diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml
@@ -121,7 +121,7 @@ jobs:
             "-DCMAKE_INSTALL_PREFIX=${InstallPrefix}" \
             "-DCMAKE_BUILD_TYPE=${BuildType}" \
             "-GNinja" \
-            ${ProtonCMakeExtraArgs}
+            ${{env.ProtonCMakeExtraArgs}}
 
       - name: qpid-proton cmake build/install
         run: cmake --build "${ProtonBuildDir}" --config ${BuildType} -t install
@@ -137,7 +137,7 @@ jobs:
             "-DCMAKE_BUILD_TYPE=${BuildType}" \
             "-DPYTHON_TEST_COMMAND='-m;pytest;-vs;--junit-prefix=pytest.\${py_test_module};--junit-xml=junitxmls/\${py_test_module}.xml;--pyargs;\${py_test_module}'" \
             "-GNinja" \
-            ${DispatchCMakeExtraArgs}
+            ${{env.DispatchCMakeExtraArgs}}
 
       - name: qpid-dispatch cmake build/install
         run: cmake --build "${DispatchBuildDir}" --config ${BuildType} -t install
diff --git a/src/dispatch.c b/src/dispatch.c
@@ -340,6 +340,7 @@ qd_error_t qd_dispatch_prepare(qd_dispatch_t *qd)
 void qd_dispatch_set_agent(qd_dispatch_t *qd, void *agent) {
     assert(agent);
     assert(!qd->agent);
+    Py_IncRef(agent);  // TODO: why not incref here? actually probably needed, I get a leak in test 9 without this, strange...
     qd->agent = agent;
 }
 
@@ -371,12 +372,18 @@ void qd_dispatch_free(qd_dispatch_t *qd)
     qd_connection_manager_free(qd->connection_manager);
     qd_policy_free(qd->policy);
     Py_XDECREF((PyObject*) qd->agent);
+//    Py_XDECREF(qd_python_module());  // hack
+    PyGC_Collect();  // run destructors while we still have the router around
+//    int ret = PyRun_SimpleString("import objgraph; import gc; gc.collect(); from qpid_dispatch_internal.management import config");
+//    assert(ret == 0);
+
+//    qd_python_finalize();
+
     qd_router_free(qd->router);
     qd_container_free(qd->container);
     qd_server_free(qd->server);
     qd_log_finalize();
-    qd_alloc_finalize();
-    qd_python_finalize();
+    qd_alloc_finalize();  // python objects may use alloc pool objects during finalization? TODO: they do that now  // actually, no, too late to do real work now
     qd_dispatch_set_router_id(qd, NULL);
     qd_dispatch_set_router_area(qd, NULL);
     qd_iterator_finalize();
diff --git a/src/python_embedded.c b/src/python_embedded.c
@@ -565,7 +565,8 @@ static PyTypeObject LogAdapterType = {
     .tp_dealloc   = (destructor)LogAdapter_dealloc,
     .tp_flags     = Py_TPFLAGS_DEFAULT,
     .tp_methods   = LogAdapter_methods,
-    .tp_init      = (initproc)LogAdapter_init
+    .tp_init      = (initproc)LogAdapter_init,
+    .tp_new       = PyType_GenericNew,
 };
 
 
@@ -719,10 +720,24 @@ static int IoAdapter_init(IoAdapter *self, PyObject *args, PyObject *kwds)
     return 0;
 }
 
+// visit all members which may conceivably participate in reference cycles
+static int IoAdapter_traverse(IoAdapter* self, visitproc visit, void *arg)
+{
+    Py_VISIT(self->handler);
+    return 0;
+}
+
+static int IoAdapter_clear(IoAdapter* self)
+{
+    Py_CLEAR(self->handler);
+    return 0;
+}
+
 static void IoAdapter_dealloc(IoAdapter* self)
 {
     qdr_core_unsubscribe(self->sub);
-    Py_DECREF(self->handler);
+    PyObject_GC_UnTrack(self);
+    IoAdapter_clear(self);
     Py_TYPE(self)->tp_free((PyObject*)self);
 }
 
@@ -802,10 +817,13 @@ static PyTypeObject IoAdapterType = {
     .tp_name      = DISPATCH_MODULE ".IoAdapter",
     .tp_doc       = "Dispatch IO Adapter",
     .tp_basicsize = sizeof(IoAdapter),
+    .tp_traverse  = (traverseproc)IoAdapter_traverse,
+    .tp_clear     = (inquiry)IoAdapter_clear,
     .tp_dealloc   = (destructor)IoAdapter_dealloc,
-    .tp_flags     = Py_TPFLAGS_DEFAULT,
+    .tp_flags     = Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_GC,
     .tp_methods   = IoAdapter_methods,
     .tp_init      = (initproc)IoAdapter_init,
+    .tp_new       = PyType_GenericNew,
 };
 
 
@@ -821,8 +839,6 @@ static void qd_register_constant(PyObject *module, const char *name, uint32_t va
 
 static void qd_python_setup(void)
 {
-    LogAdapterType.tp_new = PyType_GenericNew;
-    IoAdapterType.tp_new  = PyType_GenericNew;
     if ((PyType_Ready(&LogAdapterType) < 0) || (PyType_Ready(&IoAdapterType) < 0)) {
         qd_error_py();
         qd_log(log_source, QD_LOG_CRITICAL, "Unable to initialize Adapters");
@@ -866,7 +882,7 @@ static void qd_python_setup(void)
         qd_register_constant(m, "TREATMENT_ANYCAST_CLOSEST",  QD_TREATMENT_ANYCAST_CLOSEST);
         qd_register_constant(m, "TREATMENT_ANYCAST_BALANCED", QD_TREATMENT_ANYCAST_BALANCED);
 
-        Py_INCREF(m);
+//        Py_INCREF(m);  // PyImport_ImportModule does the increment already
         dispatch_module = m;
     }
 
diff --git a/src/router_core/router_core.c b/src/router_core/router_core.c
@@ -161,6 +161,9 @@ void qdr_core_free(qdr_core_t *core)
         }
     }
 
+    // I have to first run qdrc_endpoint_do_cleanup_CT src/router_core/core_link_endpoint.c:241
+    //  and only then qcm_edge_router_final_CT src/router_core/modules/edge_router/module.c:59
+
     qdr_link_route_t *link_route = 0;
     while ( (link_route = DEQ_HEAD(core->link_routes))) {
         DEQ_REMOVE_HEAD(core->link_routes);
@@ -323,17 +326,54 @@ void qdr_core_free(qdr_core_t *core)
     }
     assert(DEQ_SIZE(core->streaming_connections) == 0);
 
+    // this now adds unsubscribe actions, so have to do before the discard next
+    //  plus I need to run it before core mutexes are freed
+
+    qdr_modules_finalize(core);
+
+    // discard any left over actions
+
+    qdr_action_list_t  action_list;
+    DEQ_MOVE(core->action_list, action_list);
+    DEQ_APPEND(action_list, core->action_list_background);
+    qdr_action_t *action = DEQ_HEAD(action_list);
+    while (action) {
+        DEQ_REMOVE_HEAD(action_list);
+        action->action_handler(core, action, true);
+        free_qdr_action_t(action);
+        action = DEQ_HEAD(action_list);
+    }
+
+    // Drain the general work lists
+    qdr_general_handler(core);
+
     // at this point all the conn identifiers have been freed
     qd_hash_free(core->conn_id_hash);
 
     qdr_agent_free(core->mgmt_agent);
 
+    //
+    // Free the core resources
+    //
     if (core->routers_by_mask_bit)       free(core->routers_by_mask_bit);
     if (core->control_links_by_mask_bit) free(core->control_links_by_mask_bit);
     if (core->data_links_by_mask_bit)    free(core->data_links_by_mask_bit);
     if (core->neighbor_free_mask)        qd_bitmask_free(core->neighbor_free_mask);
     if (core->rnode_conns_by_mask_bit)   free(core->rnode_conns_by_mask_bit);
 
+    sys_thread_free(core->thread);
+    sys_cond_free(core->action_cond);
+    sys_mutex_free(core->action_lock);
+    sys_mutex_free(core->work_lock);
+    sys_mutex_free(core->id_lock);
+    qd_timer_free(core->work_timer);
+
+    for (int i = 0; i <= QD_TREATMENT_LINK_BALANCED; ++i) {
+        if (core->forwarders[i]) {
+            free(core->forwarders[i]);
+        }
+    }
+
     free(core);
 }
 
diff --git a/src/router_node.c b/src/router_node.c
@@ -2169,12 +2169,12 @@ void qd_router_free(qd_router_t *router)
 
     qd_container_set_default_node_type(router->qd, 0, 0, QD_DIST_BOTH);
 
+    qd_router_python_free(router);
     qdr_core_free(router->router_core);
     qd_tracemask_free(router->tracemask);
     qd_timer_free(router->timer);
     sys_mutex_free(router->lock);
     qd_router_configure_free(router);
-    qd_router_python_free(router);
 
     free(router);
     qd_router_id_finalize();
diff --git a/src/router_pynode.c b/src/router_pynode.c
@@ -308,11 +308,12 @@ static PyMethodDef RouterAdapter_methods[] = {
 
 static PyTypeObject RouterAdapterType = {
     PyVarObject_HEAD_INIT(NULL, 0)
-    .tp_name = "dispatch.RouterAdapter",  /* tp_name*/
-    .tp_basicsize = sizeof(RouterAdapter),     /* tp_basicsize*/
-    .tp_flags = Py_TPFLAGS_DEFAULT,        /* tp_flags*/
-    .tp_doc = "Dispatch Router Adapter", /* tp_doc */
-    .tp_methods = RouterAdapter_methods,     /* tp_methods */
+    .tp_name = "dispatch.RouterAdapter",
+    .tp_basicsize = sizeof(RouterAdapter),
+    .tp_flags = Py_TPFLAGS_DEFAULT,
+    .tp_doc = "Dispatch Router Adapter",
+    .tp_methods = RouterAdapter_methods,
+    .tp_new = PyType_GenericNew,
 };
 
 
@@ -393,7 +394,6 @@ qd_error_t qd_router_python_setup(qd_router_t *router)
         return QD_ERROR_NONE;
 
     PyObject *pDispatchModule = qd_python_module();
-    RouterAdapterType.tp_new = PyType_GenericNew;
     PyType_Ready(&RouterAdapterType);
     QD_ERROR_PY_RET();
 
@@ -443,6 +443,7 @@ qd_error_t qd_router_python_setup(qd_router_t *router)
     // Instantiate the router
     //
     pyRouter = PyObject_CallObject(pClass, pArgs);
+    Py_DECREF(pClass);
     Py_DECREF(pArgs);
     Py_DECREF(adapterType);
     QD_ERROR_PY_RET();
@@ -455,7 +456,12 @@ qd_error_t qd_router_python_setup(qd_router_t *router)
 }
 
 void qd_router_python_free(qd_router_t *router) {
-    // empty
+    Py_XDECREF(pyRouter);
+    Py_XDECREF(pyTick);
+    Py_XDECREF(pySetMobileSeq);
+    Py_XDECREF(pySetMyMobileSeq);
+    Py_XDECREF(pyLinkLost);
+//    qd_python_finalize();
 }
 
 
diff --git a/tests/lsan.supp b/tests/lsan.supp
@@ -2,9 +2,6 @@
 # found by AddressSanitizer (ASAN)
 #
 
-# to be triaged; pretty much all tests
-leak:^IoAdapter_init$
-
 # to be triaged; pretty much all tests
 leak:^load_server_config$
 

Original file line number	Diff line number	Diff line change
`@@ -2,9 +2,6 @@`
`2`	`2`	`# found by AddressSanitizer (ASAN)`
`3`	`3`	`#`
`4`	`4`
`5`		`-# to be triaged; pretty much all tests`
`6`		`-leak:^IoAdapter_init$`
`7`		`-`
`8`	`5`	`# to be triaged; pretty much all tests`
`9`	`6`	`leak:^load_server_config$`
`10`	`7`