From 071c655fd314b5da4eb572aabf02635c26be5742 Mon Sep 17 00:00:00 2001 From: Jiri Danek Date: Mon, 15 Feb 2021 10:45:50 +0100 Subject: [PATCH] DISPATCH-1962 Fix leak of qdr_core_subscribe 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 f486435d 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 f486435d 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 f486435d 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 --- .github/workflows/build.yaml | 4 ++-- src/dispatch.c | 11 ++++++++-- src/python_embedded.c | 28 ++++++++++++++++++------ src/router_core/router_core.c | 40 +++++++++++++++++++++++++++++++++++ src/router_node.c | 2 +- src/router_pynode.c | 20 ++++++++++++------ tests/lsan.supp | 3 --- 7 files changed, 87 insertions(+), 21 deletions(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index a62e041262..02bb5573b7 100644 --- 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 index f19339b414..1b41cb61ed 100644 --- 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 index 896516efcb..b60b767201 100644 --- 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 index 54da952eb5..b9622717e3 100644 --- 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 index a9fa0481bb..dfa2de4caa 100644 --- 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 index a624bc6c5c..33b4ffe038 100644 --- 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 index 39174f804e..95332e6c18 100644 --- 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$