You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by GitBox <gi...@apache.org> on 2021/02/15 09:54:40 UTC

[GitHub] [qpid-dispatch] jiridanek opened a new pull request #1032: Make lsan suppressions more specific and fix revealed leaks

jiridanek opened a new pull request #1032:
URL: https://github.com/apache/qpid-dispatch/pull/1032


   I did not finish with making this leak free yet. It is quite difficult for me to predict what happens as I make (pretty significant :( changes all around... Currently, my biggest problem is that after I allow Python to collect the circular object subgraph including IoAdapter, the collection is happening too late in the shutdown, where I cannot no longer schedule actions:
   
   ```
   14: ==30770==ERROR: AddressSanitizer: heap-use-after-free on address 0x61a00003a8f0 at pc 0x7f321b4e7dae bp 0x7ffd94518bd0 sp 0x7ffd94518bc8
   14: READ of size 8 at 0x61a00003a8f0 thread T0
   14:     #0 0x7f321b4e7dad in qdr_action_enqueue ../src/router_core/router_core.c:399
   14:     #1 0x7f321b5043db in qdr_core_unsubscribe ../src/router_core/route_tables.c:173
   14:     #2 0x7f321b453b0c in IoAdapter_dealloc ../src/python_embedded.c:737
   14:     #3 0x7f321ac861ad in list_dealloc (/nix/store/0yhk4sk4x9s9hsrf3p1skbfy1pwd1rbf-python3-3.8.5/lib/libpython3.8.so.1.0+0x1371ad)
   14:     #4 0x7f321ac9e59a in dict_dealloc (/nix/store/0yhk4sk4x9s9hsrf3p1skbfy1pwd1rbf-python3-3.8.5/lib/libpython3.8.so.1.0+0x14f59a)
   14:     #5 0x7f321acb8138 in subtype_clear (/nix/store/0yhk4sk4x9s9hsrf3p1skbfy1pwd1rbf-python3-3.8.5/lib/libpython3.8.so.1.0+0x169138)
   14:     #6 0x7f321ad115d0 in collect.constprop.0 (/nix/store/0yhk4sk4x9s9hsrf3p1skbfy1pwd1rbf-python3-3.8.5/lib/libpython3.8.so.1.0+0x1c25d0)
   14:     #7 0x7f321ad72670 in collect_with_callback.constprop.0 (/nix/store/0yhk4sk4x9s9hsrf3p1skbfy1pwd1rbf-python3-3.8.5/lib/libpython3.8.so.1.0+0x223670)
   14:     #8 0x7f321ad7275d in PyGC_Collect (/nix/store/0yhk4sk4x9s9hsrf3p1skbfy1pwd1rbf-python3-3.8.5/lib/libpython3.8.so.1.0+0x22375d)
   14:     #9 0x7f321ad68d0b in Py_FinalizeEx (/nix/store/0yhk4sk4x9s9hsrf3p1skbfy1pwd1rbf-python3-3.8.5/lib/libpython3.8.so.1.0+0x219d0b)
   14:     #10 0x7f321b454cc6 in qd_python_finalize ../src/python_embedded.c:73
   14:     #11 0x7f321b3fb0f1 in qd_dispatch_free ../src/dispatch.c:388
   14:     #12 0x402625 in main_process ../router/src/main.c:117
   14:     #13 0x403f4b in main ../router/src/main.c:367
   14:     #14 0x7f321a047cbc in __libc_start_main (/nix/store/q53f5birhik4dxg3q3r2g5f324n7r5mc-glibc-2.31-74/lib/libc.so.6+0x23cbc)
   14:     #15 0x402419 in _start (/home/jdanek/repos/qpid/qpid-dispatch/cmake-build-debug/router/qdrouterd+0x402419)
   ```
   
   I could not figure out what is keeping the object subgraph alive so that it does not get collected sooner, when I want it to be destroyed.
   
   Also, the suppressions need to be tried on all supported platforms, because each Python version, etc. can leak differently. And as the Jira shows, suppressing all leaky traces that include Python is not a good solution, because it suppresses too much (incl. the IoAdapter leak; admittedly not super-serious, but this is a matter of principle! `)


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] jiridanek commented on pull request #1032: Make lsan suppressions more specific and fix revealed leaks

Posted by GitBox <gi...@apache.org>.
jiridanek commented on pull request #1032:
URL: https://github.com/apache/qpid-dispatch/pull/1032#issuecomment-779104103


   I'll revisit this next weekend and try to make some more sense of ^^^


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] jiridanek commented on a change in pull request #1032: Make lsan suppressions more specific and fix revealed leaks

Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #1032:
URL: https://github.com/apache/qpid-dispatch/pull/1032#discussion_r576085232



##########
File path: src/server.c
##########
@@ -1372,6 +1372,10 @@ qd_server_t *qd_server(qd_dispatch_t *qd, int thread_count, const char *containe
 void qd_server_free(qd_server_t *qd_server)
 {
     if (!qd_server) return;
+
+    qd_http_server_stop(qd_server->http); /* Stop HTTP threads immediately */
+    qd_http_server_free(qd_server->http);

Review comment:
       this is so that unittests can stop the http server, which prevents a leak from the test code. stopping it in qd_server_run does not give unittests an option to call this




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] jiridanek commented on a change in pull request #1032: Make lsan suppressions more specific and fix revealed leaks

Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #1032:
URL: https://github.com/apache/qpid-dispatch/pull/1032#discussion_r576074879



##########
File path: src/python_embedded.c
##########
@@ -718,10 +719,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;
+}
+

Review comment:
       This deals with the following leak, which was previously suppressed, because it involves Python in the stacktrace
   
   ```
   9: Direct leak of 56 byte(s) in 1 object(s) allocated from:
   9:     #0 0x7f78a3606e8f in __interceptor_malloc (/nix/store/g40sl3zh3nv52vj0mrl4iki5iphh5ika-gcc-10.2.0-lib/lib/libasan.so.6+0xace8f)
   9:     #1 0x7f78a2d64afb in qd_malloc ../include/qpid/dispatch/ctools.h:229
   9:     #2 0x7f78a2d657da in qdr_core_subscribe ../src/router_core/route_tables.c:149
   9:     #3 0x7f78a2c83072 in IoAdapter_init ../src/python_embedded.c:711
   9:     #4 0x7f78a2353a6c in type_call (/nix/store/r85nxfnwiv45nbmf5yb60jj8ajim4m7w-python3-3.8.5/lib/libpython3.8.so.1.0+0x165a6c)
   ```
   
   The problem is in
   
   ```python
   class Agent:
       ...
       def activate(self, address):
           ...
           self.io = IoAdapter(self.receive, address, 'L', '0', TREATMENT_ANYCAST_CLOSEST)
   ```
   
   IoAdapter refers to Agent (through the bound method reference to self.receive) and Agent refers to IoAdapter (through property self.io). Since IoAdapter is implemented in C and does not support Python's GC, there is no way to break the cycle.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] jiridanek commented on a change in pull request #1032: Make lsan suppressions more specific and fix revealed leaks

Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #1032:
URL: https://github.com/apache/qpid-dispatch/pull/1032#discussion_r576084319



##########
File path: src/router_pynode.c
##########
@@ -307,11 +307,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,

Review comment:
       for some reason, the code is always setting this separately afterwards; I think there is not real advantage to that (?) and this way is cleaner




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] jiridanek commented on a change in pull request #1032: Make lsan suppressions more specific and fix revealed leaks

Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #1032:
URL: https://github.com/apache/qpid-dispatch/pull/1032#discussion_r576076081



##########
File path: src/router_core/modules/mobile_sync/mobile.c
##########
@@ -934,11 +934,13 @@ static void qcm_mobile_sync_final_CT(void *module_context)
 {
     qdrm_mobile_sync_t *msync = (qdrm_mobile_sync_t*) module_context;
 
+    qdr_core_unsubscribe(msync->message_sub1);
+    qdr_core_unsubscribe(msync->message_sub2);

Review comment:
       ```
   72: Direct leak of 56 byte(s) in 1 object(s) allocated from:
   72:     #0 0x7f2f3dec0e8f in __interceptor_malloc (/nix/store/g40sl3zh3nv52vj0mrl4iki5iphh5ika-gcc-10.2.0-lib/lib/libasan.so.6+0xace8f)
   72:     #1 0x7f2f3d61ebe8 in qd_malloc ../include/qpid/dispatch/ctools.h:229
   72:     #2 0x7f2f3d61f8c7 in qdr_core_subscribe ../src/router_core/route_tables.c:149
   72:     #3 0x7f2f3d689657 in qcm_mobile_sync_init_CT ../src/router_core/modules/mobile_sync/mobile.c:919
   72:     #4 0x7f2f3d61bc6f in qdr_modules_init ../src/router_core/router_core_thread.c:120
   72:     #5 0x7f2f3d5fbae7 in qdr_core_setup_init ../src/router_core/router_core.c:60
   72:     #6 0x7f2f3d5fcae9 in qdr_core ../src/router_core/router_core.c:116
   72:     #7 0x7f2f3d69ad1e in qd_router_setup_late ../src/router_node.c:2072
   72:     #8 0x7f2f3825aabc in ffi_call_unix64 (/nix/store/m8y5mz1f0al3rg3b56rq5bza49jjxnc0-libffi-3.3/lib/libffi.so.7+0x7abc)
   72:     #9 0x7ffec59223ef  ([stack]+0x1f3ef)
   72: 
   72: Direct leak of 56 byte(s) in 1 object(s) allocated from:
   72:     #0 0x7f2f3dec0e8f in __interceptor_malloc (/nix/store/g40sl3zh3nv52vj0mrl4iki5iphh5ika-gcc-10.2.0-lib/lib/libasan.so.6+0xace8f)
   72:     #1 0x7f2f3d61ebe8 in qd_malloc ../include/qpid/dispatch/ctools.h:229
   72:     #2 0x7f2f3d61f8c7 in qdr_core_subscribe ../src/router_core/route_tables.c:149
   72:     #3 0x7f2f3d689705 in qcm_mobile_sync_init_CT ../src/router_core/modules/mobile_sync/mobile.c:921
   72:     #4 0x7f2f3d61bc6f in qdr_modules_init ../src/router_core/router_core_thread.c:120
   72:     #5 0x7f2f3d5fbae7 in qdr_core_setup_init ../src/router_core/router_core.c:60
   72:     #6 0x7f2f3d5fcae9 in qdr_core ../src/router_core/router_core.c:116
   72:     #7 0x7f2f3d69ad1e in qd_router_setup_late ../src/router_node.c:2072
   72:     #8 0x7f2f3825aabc in ffi_call_unix64 (/nix/store/m8y5mz1f0al3rg3b56rq5bza49jjxnc0-libffi-3.3/lib/libffi.so.7+0x7abc)
   72:     #9 0x7ffec59223ef  ([stack]+0x1f3ef)
   ```




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] jiridanek commented on a change in pull request #1032: Make lsan suppressions more specific and fix revealed leaks

Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #1032:
URL: https://github.com/apache/qpid-dispatch/pull/1032#discussion_r576086187



##########
File path: tests/lsan.supp
##########
@@ -11,27 +11,59 @@ leak:qd_policy_open_fetch_settings
 leak:qdr_error_description
 
 # DISPATCH-1844 - shutdown leak
-leak:sys_mutex
+#leak:sys_mutex
 
 # expected, not a bug:
 #
-leak:qdr_core_subscribe
+#leak:qdr_core_subscribe
 
 # Ubuntu 16.04 (Xenial)
 #
 leak:_ctypes_alloc_format_string
 leak:__strdup
 
+# to be triaged; system_tests_http
+leak:^callback_healthz$
+leak:^callback_metrics$
+
+# to be triaged; system_tests_http1_adaptor
+leak:^pn_condition$
+leak:^pn_raw_connection$
+
 ####
-#### Miscellaneous 3rd party libraries, test code, etc:
+#### Miscellaneous 3rd party libraries:
 ####
 
-leak:*libpython*
-leak:*libwebsockets*
-leak:*python2*
+# these leaks happen even after simple Py_Initialize(); Py_Finalize();
+#  https://bugs.python.org/issue1635741
+leak:^_PyObject_Realloc
+leak:^PyObject_Malloc$
+leak:^PyThread_allocate_lock$
+
+# the PyMalloc mechanism is incompatible with Valgrind, it must be disabled or reported "leaks" must be suppressed
+#  https://pythonextensionpatterns.readthedocs.io/en/latest/debugging/debug_python.html#debug-version-of-python-memory-alloc-label
+leak:^PyMem_Malloc$
+leak:^_PyObject_GC_Resize$
+# Python uses these alloc functions if you define PYTHONDEVMODE=1
+leak:^_PyMem_DebugRawAlloc$
+leak:^_PyMem_DebugRawRealloc$
+# All the rest
+leak:^list_append$
 
-# We should be able to uncomment these once all known dispatch leaks have been fixed
-leak:*libqpid-proton*
+# Wholesale library suppressions, avoid doing this if at all possible
+#  one reason we may not be able to avoid doing this is compatibility with multiple versions of libs on many operating systems
+#leak:/libpython2.*.so
+#leak:/libpython3.*.so
+#leak:/libwebsockets.so
+#leak:libqpid-proton
 
-# Ignore test code
-leak:run_unit_tests.c
+# We might be able to remove these once all known dispatch leaks have been fixed
+# Suppressions taken from Proton's lsan.supp
+#  this appears in system_tests_open_properties:
+leak:^pni_data_grow$
+leak:^pn_buffer_ensure$
+#  this appears in system_tests_http1_adaptor
+leak:^pn_string_grow$
+leak:^pn_object_new$
+leak:^pn_list$
+leak:^pni_record_create$

Review comment:
       these are known leaks in Proton, taken straight out from its lsan.supp. I think it is reasonable to carry this here for now, assuming the proton leaks get fixed in the foreseeable future




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] jiridanek commented on a change in pull request #1032: Make lsan suppressions more specific and fix revealed leaks

Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #1032:
URL: https://github.com/apache/qpid-dispatch/pull/1032#discussion_r808904712



##########
File path: src/python_embedded.c
##########
@@ -718,10 +719,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;
+}
+

Review comment:
       https://github.com/apache/qpid-dispatch/pull/1052




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] jiridanek commented on a change in pull request #1032: Make lsan suppressions more specific and fix revealed leaks

Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #1032:
URL: https://github.com/apache/qpid-dispatch/pull/1032#discussion_r576084319



##########
File path: src/router_pynode.c
##########
@@ -307,11 +307,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,

Review comment:
       for some reason, the code is always setting `tp_new` separately afterwards; I think there is not real advantage to that (?) and this way is cleaner




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] jiridanek commented on pull request #1032: Make lsan suppressions more specific and fix revealed leaks

Posted by GitBox <gi...@apache.org>.
jiridanek commented on pull request #1032:
URL: https://github.com/apache/qpid-dispatch/pull/1032#issuecomment-782708078


   Superseded by #1048 and other PRs still waiting to be proposed. I'll leave this open for a while longer, until the other PRs get processed.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] codecov-io edited a comment on pull request #1032: Make lsan suppressions more specific and fix revealed leaks

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #1032:
URL: https://github.com/apache/qpid-dispatch/pull/1032#issuecomment-779173075


   # [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1032?src=pr&el=h1) Report
   > Merging [#1032](https://codecov.io/gh/apache/qpid-dispatch/pull/1032?src=pr&el=desc) (3f360b4) into [master](https://codecov.io/gh/apache/qpid-dispatch/commit/ce7c5a0cddb89fb32c80ebf7e3773d9a9e6ac973?el=desc) (ce7c5a0) will **increase** coverage by `0.06%`.
   > The diff coverage is `100.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/qpid-dispatch/pull/1032/graphs/tree.svg?width=650&height=150&src=pr&token=rk2Cgd27pP)](https://codecov.io/gh/apache/qpid-dispatch/pull/1032?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #1032      +/-   ##
   ==========================================
   + Coverage   82.41%   82.47%   +0.06%     
   ==========================================
     Files         111      111              
     Lines       27315    27331      +16     
   ==========================================
   + Hits        22512    22542      +30     
   + Misses       4803     4789      -14     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/qpid-dispatch/pull/1032?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [src/dispatch.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1032/diff?src=pr&el=tree#diff-c3JjL2Rpc3BhdGNoLmM=) | `83.80% <100.00%> (+0.07%)` | :arrow_up: |
   | [src/python\_embedded.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1032/diff?src=pr&el=tree#diff-c3JjL3B5dGhvbl9lbWJlZGRlZC5j) | `68.28% <100.00%> (-0.15%)` | :arrow_down: |
   | [src/router\_core/modules/mobile\_sync/mobile.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1032/diff?src=pr&el=tree#diff-c3JjL3JvdXRlcl9jb3JlL21vZHVsZXMvbW9iaWxlX3N5bmMvbW9iaWxlLmM=) | `92.21% <100.00%> (+0.03%)` | :arrow_up: |
   | [src/router\_core/router\_core.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1032/diff?src=pr&el=tree#diff-c3JjL3JvdXRlcl9jb3JlL3JvdXRlcl9jb3JlLmM=) | `86.20% <100.00%> (ø)` | |
   | [src/router\_node.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1032/diff?src=pr&el=tree#diff-c3JjL3JvdXRlcl9ub2RlLmM=) | `94.12% <100.00%> (-0.11%)` | :arrow_down: |
   | [src/router\_pynode.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1032/diff?src=pr&el=tree#diff-c3JjL3JvdXRlcl9weW5vZGUuYw==) | `88.39% <100.00%> (+0.42%)` | :arrow_up: |
   | [src/server.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1032/diff?src=pr&el=tree#diff-c3JjL3NlcnZlci5j) | `86.67% <100.00%> (ø)` | |
   | [tests/core\_timer\_test.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1032/diff?src=pr&el=tree#diff-dGVzdHMvY29yZV90aW1lcl90ZXN0LmM=) | `90.47% <100.00%> (+0.09%)` | :arrow_up: |
   | [src/router\_core/forwarder.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1032/diff?src=pr&el=tree#diff-c3JjL3JvdXRlcl9jb3JlL2ZvcndhcmRlci5j) | `92.64% <0.00%> (-0.40%)` | :arrow_down: |
   | ... and [6 more](https://codecov.io/gh/apache/qpid-dispatch/pull/1032/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1032?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1032?src=pr&el=footer). Last update [ce7c5a0...3f360b4](https://codecov.io/gh/apache/qpid-dispatch/pull/1032?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] codecov-commenter commented on pull request #1032: Make lsan suppressions more specific and fix revealed leaks

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1032:
URL: https://github.com/apache/qpid-dispatch/pull/1032#issuecomment-823187222


   # [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1032?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#1032](https://codecov.io/gh/apache/qpid-dispatch/pull/1032?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (979481a) into [main](https://codecov.io/gh/apache/qpid-dispatch/commit/b919a638e75ce42b0561b70671127b4e51f95d28?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b919a63) will **increase** coverage by `0.00%`.
   > The diff coverage is `100.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/qpid-dispatch/pull/1032/graphs/tree.svg?width=650&height=150&src=pr&token=rk2Cgd27pP&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/qpid-dispatch/pull/1032?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@           Coverage Diff           @@
   ##             main    #1032   +/-   ##
   =======================================
     Coverage   84.22%   84.23%           
   =======================================
     Files         111      111           
     Lines       27569    27580   +11     
   =======================================
   + Hits        23220    23232   +12     
   + Misses       4349     4348    -1     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/qpid-dispatch/pull/1032?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [src/dispatch.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1032/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL2Rpc3BhdGNoLmM=) | `83.80% <100.00%> (+0.07%)` | :arrow_up: |
   | [src/python\_embedded.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1032/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3B5dGhvbl9lbWJlZGRlZC5j) | `68.28% <100.00%> (-0.15%)` | :arrow_down: |
   | [src/router\_core/modules/mobile\_sync/mobile.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1032/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9jb3JlL21vZHVsZXMvbW9iaWxlX3N5bmMvbW9iaWxlLmM=) | `92.21% <100.00%> (+0.03%)` | :arrow_up: |
   | [src/router\_core/router\_core.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1032/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9jb3JlL3JvdXRlcl9jb3JlLmM=) | `85.19% <100.00%> (-0.67%)` | :arrow_down: |
   | [src/router\_node.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1032/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9ub2RlLmM=) | `92.94% <100.00%> (+0.19%)` | :arrow_up: |
   | [src/router\_pynode.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1032/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9weW5vZGUuYw==) | `88.23% <100.00%> (+0.27%)` | :arrow_up: |
   | [...c/router\_core/modules/test\_hooks/core\_test\_hooks.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1032/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9jb3JlL21vZHVsZXMvdGVzdF9ob29rcy9jb3JlX3Rlc3RfaG9va3MuYw==) | `92.03% <0.00%> (-1.28%)` | :arrow_down: |
   | [src/router\_core/connections.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1032/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9jb3JlL2Nvbm5lY3Rpb25zLmM=) | `89.31% <0.00%> (-0.30%)` | :arrow_down: |
   | ... and [8 more](https://codecov.io/gh/apache/qpid-dispatch/pull/1032/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1032?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1032?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [b919a63...979481a](https://codecov.io/gh/apache/qpid-dispatch/pull/1032?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] jiridanek commented on a change in pull request #1032: Make lsan suppressions more specific and fix revealed leaks

Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #1032:
URL: https://github.com/apache/qpid-dispatch/pull/1032#discussion_r576087201



##########
File path: tests/lsan.supp
##########
@@ -11,27 +11,59 @@ leak:qd_policy_open_fetch_settings
 leak:qdr_error_description
 
 # DISPATCH-1844 - shutdown leak
-leak:sys_mutex
+#leak:sys_mutex
 
 # expected, not a bug:
 #
-leak:qdr_core_subscribe
+#leak:qdr_core_subscribe
 
 # Ubuntu 16.04 (Xenial)
 #
 leak:_ctypes_alloc_format_string
 leak:__strdup
 
+# to be triaged; system_tests_http
+leak:^callback_healthz$
+leak:^callback_metrics$

Review comment:
       if I understand correctly, these leaks would be per-request, not single leak per run; but I did not investigate this yet
   
   one of the leaks hidden by too-broad suppressions




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] jiridanek commented on a change in pull request #1032: Make lsan suppressions more specific and fix revealed leaks

Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #1032:
URL: https://github.com/apache/qpid-dispatch/pull/1032#discussion_r576933871



##########
File path: src/router_pynode.c
##########
@@ -452,11 +456,17 @@ qd_error_t qd_router_python_setup(qd_router_t *router)
     pySetMobileSeq   = PyObject_GetAttrString(pyRouter, "setMobileSeq"); QD_ERROR_PY_RET();
     pySetMyMobileSeq = PyObject_GetAttrString(pyRouter, "setMyMobileSeq"); QD_ERROR_PY_RET();
     pyLinkLost       = PyObject_GetAttrString(pyRouter, "linkLost"); QD_ERROR_PY_RET();
+//    Py_DECREF(adapterInstance);  // TODO: why not this? get python exceptions if I try

Review comment:
       I understand now. `PyTuple_SetItem` does not incref, it transfers ownership from local context into the tuple. Meaning that doing decref here (or for the other tuple items before) is a mistake.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] codecov-io commented on pull request #1032: Make lsan suppressions more specific and fix revealed leaks

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #1032:
URL: https://github.com/apache/qpid-dispatch/pull/1032#issuecomment-779173075


   # [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1032?src=pr&el=h1) Report
   > Merging [#1032](https://codecov.io/gh/apache/qpid-dispatch/pull/1032?src=pr&el=desc) (b24d03a) into [master](https://codecov.io/gh/apache/qpid-dispatch/commit/ce7c5a0cddb89fb32c80ebf7e3773d9a9e6ac973?el=desc) (ce7c5a0) will **increase** coverage by `0.06%`.
   > The diff coverage is `100.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/qpid-dispatch/pull/1032/graphs/tree.svg?width=650&height=150&src=pr&token=rk2Cgd27pP)](https://codecov.io/gh/apache/qpid-dispatch/pull/1032?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #1032      +/-   ##
   ==========================================
   + Coverage   82.41%   82.48%   +0.06%     
   ==========================================
     Files         111      111              
     Lines       27315    27330      +15     
   ==========================================
   + Hits        22512    22542      +30     
   + Misses       4803     4788      -15     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/qpid-dispatch/pull/1032?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [src/dispatch.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1032/diff?src=pr&el=tree#diff-c3JjL2Rpc3BhdGNoLmM=) | `83.80% <100.00%> (+0.07%)` | :arrow_up: |
   | [src/python\_embedded.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1032/diff?src=pr&el=tree#diff-c3JjL3B5dGhvbl9lbWJlZGRlZC5j) | `68.28% <100.00%> (-0.15%)` | :arrow_down: |
   | [src/router\_core/modules/mobile\_sync/mobile.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1032/diff?src=pr&el=tree#diff-c3JjL3JvdXRlcl9jb3JlL21vZHVsZXMvbW9iaWxlX3N5bmMvbW9iaWxlLmM=) | `92.21% <100.00%> (+0.03%)` | :arrow_up: |
   | [src/router\_core/router\_core.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1032/diff?src=pr&el=tree#diff-c3JjL3JvdXRlcl9jb3JlL3JvdXRlcl9jb3JlLmM=) | `86.20% <100.00%> (ø)` | |
   | [src/router\_node.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1032/diff?src=pr&el=tree#diff-c3JjL3JvdXRlcl9ub2RlLmM=) | `94.12% <100.00%> (-0.11%)` | :arrow_down: |
   | [src/router\_pynode.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1032/diff?src=pr&el=tree#diff-c3JjL3JvdXRlcl9weW5vZGUuYw==) | `88.39% <100.00%> (+0.42%)` | :arrow_up: |
   | [src/server.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1032/diff?src=pr&el=tree#diff-c3JjL3NlcnZlci5j) | `86.76% <100.00%> (+0.09%)` | :arrow_up: |
   | [tests/core\_timer\_test.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1032/diff?src=pr&el=tree#diff-dGVzdHMvY29yZV90aW1lcl90ZXN0LmM=) | `90.47% <100.00%> (+0.09%)` | :arrow_up: |
   | [...c/router\_core/modules/test\_hooks/core\_test\_hooks.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1032/diff?src=pr&el=tree#diff-c3JjL3JvdXRlcl9jb3JlL21vZHVsZXMvdGVzdF9ob29rcy9jb3JlX3Rlc3RfaG9va3MuYw==) | `92.03% <0.00%> (-1.28%)` | :arrow_down: |
   | ... and [10 more](https://codecov.io/gh/apache/qpid-dispatch/pull/1032/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1032?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1032?src=pr&el=footer). Last update [ce7c5a0...3f360b4](https://codecov.io/gh/apache/qpid-dispatch/pull/1032?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] jiridanek commented on a change in pull request #1032: Make lsan suppressions more specific and fix revealed leaks

Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #1032:
URL: https://github.com/apache/qpid-dispatch/pull/1032#discussion_r576934797



##########
File path: src/router_pynode.c
##########
@@ -452,11 +456,17 @@ qd_error_t qd_router_python_setup(qd_router_t *router)
     pySetMobileSeq   = PyObject_GetAttrString(pyRouter, "setMobileSeq"); QD_ERROR_PY_RET();
     pySetMyMobileSeq = PyObject_GetAttrString(pyRouter, "setMyMobileSeq"); QD_ERROR_PY_RET();
     pyLinkLost       = PyObject_GetAttrString(pyRouter, "linkLost"); QD_ERROR_PY_RET();
+//    Py_DECREF(adapterInstance);  // TODO: why not this? get python exceptions if I try

Review comment:
       http://www.cse.psu.edu/~gxt29/papers/refcount.pdf: "When programmers use thoseAPI functions, they can often be confusedby their effects on refcounts and makemistakes." I can see why that might be the case.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] jiridanek edited a comment on pull request #1032: Make lsan suppressions more specific and fix revealed leaks

Posted by GitBox <gi...@apache.org>.
jiridanek edited a comment on pull request #1032:
URL: https://github.com/apache/qpid-dispatch/pull/1032#issuecomment-779104103


   I'll revisit this next weekend and try to make some more sense of ^^^
   
   edit: it would probably make the most sense to do this in two stages, first rewrite suppression file (so that CI stays green), then address suppressions one by one.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org