-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BUG]: Potential leaks when using enum_ #3865
Comments
Likely causes include our abbusive use of attr to add values to the dictionary. The fact that we are storing a C++ pair in the python dictionary, or finally some bug with dynamic_attrs that we haven't caught. It could also be a reference cycle between the attr and the enum, in which case Python won't properly GC when the class is deleted. |
Could you explain how do you know there is a leak? |
Memory leaks or errors seems to be reported by your favorite memory checker (Totalview, valgrind, asan ...) if you build the pybind module with debugging options :
According to the reports, the faulty lines seems to be
Context of call varies, for example here one of the stacks (sorry for readability !)
|
How did you get that? I did as follows
|
That's great, it means you compiled with gcc sanitizer sucessfully :) (that is the
Be careful :
|
I also show leaks with asan only for enum (every enum) in make_function_record() |
@cjolivier01 Could this be related to the free_data function not being set correctly? A relevant PR seems like #3229 which fixed a similar memory leak. Also could be related to the InitializingFunctionRecordDeleter in pybind11 perhaps. |
We're also seeing these leaks with enums. Also, if you add the |
Part of OpenAssetIO#739. We were combining the "bridge" tests of the `openassetio-python-bridge` with the "module" tests of Python bindings helpers in `openassetio-python-module` and, somewhat arbitrarily, grouping them under an "internal" heading. These tests all relied on running under `pytest` and using an `_openassetio_test` Python extension module to provide C++ helpers. This makes sense for the "module" tests (`PyRetainingSharedPtr`) but was a quick-and-dirty solution for the "bridge" tests (`createPythonPluginSystemManagerImplementationFactory`). So move the "bridge" tests out into their own directory with a C++ CTest that embeds a Python interpreter. This is much more representative of how we expect the `openassetio-python-bridge` library to be used. The leak sanitizer (via ASan) flags some leaks from pybind11 when the test program exits. This is apparently a known bug (pybind/pybind11#3865). The leak arises when defining a `py::enum_` and then calling `.value` on it to add an entry to the enum. It's mysterious that we've not seen it in previous sanitizer runs that exercise the offending part of the code (i.e. `pytest` tests that import the `_openassetio` bindings). It could have been hidden behind the `python` executable, whereas for this test we have a custom `openassetio-python-bridge-test-exe` executable that embeds Python. Or we could be missing some cleanup in this executable, which `python` handles properly. Assuming this is a genuine bug, at least it's only a leak at program exit, so is benign, and there's nothing we can do about it until pybind11 fixes it upstream. So just suppress any leaks from being reported if they have `pybind11::enum_<` in their stack trace. Signed-off-by: David Feltell <david.feltell@foundry.com>
Part of OpenAssetIO#739. We were combining the "bridge" tests of the `openassetio-python-bridge` with the "module" tests of Python bindings helpers in `openassetio-python-module` and, somewhat arbitrarily, grouping them under an "internal" heading. These tests all relied on running under `pytest` and using an `_openassetio_test` Python extension module to provide C++ helpers. This makes sense for the "module" tests (`PyRetainingSharedPtr`) but was a quick-and-dirty solution for the "bridge" tests (`createPythonPluginSystemManagerImplementationFactory`). So move the "bridge" tests out into their own directory with a C++ CTest that embeds a Python interpreter. This is much more representative of how we expect the `openassetio-python-bridge` library to be used. The leak sanitizer (via ASan) flags some leaks from pybind11 when the test program exits. This is apparently a known bug (pybind/pybind11#3865). The leak arises when defining a `py::enum_` and then calling `.value` on it to add an entry to the enum. It's mysterious that we've not seen it in previous sanitizer runs that exercise the offending part of the code (i.e. `pytest` tests that import the `_openassetio` bindings). It could have been hidden behind the `python` executable, whereas for this test we have a custom `openassetio-python-bridge-test-exe` executable that embeds Python. Or we could be missing some cleanup in this executable, which `python` handles properly. Assuming this is a genuine bug, at least it's only a leak at program exit, so is benign, and there's nothing we can do about it until pybind11 fixes it upstream. So just suppress any leaks from being reported if they have `pybind11::enum_<` in their stack trace. Signed-off-by: David Feltell <david.feltell@foundry.com>
After doing some more investigation, I am fairly sure it's due to use calling .attr() on enum_base which is just a handle of the object store in a separate struct of enum in a weird way. We probably need to refactor to get rid of the weird base struct in a so that the attributes are properly cleared. Something is keeping the attrs we add during construction from being 'dec_ref' properly |
* Ignore leaks detected in: * "/usr/bin" e.g. bash and python * "site-packages/" e.g. numpy * "pybind11.h" * Currently having an issue with leaking enum bindings * pybind/pybind11#3865 Signed-off-by: Jared Duffey <jared.duffey@bluequartz.net>
* Ignore leaks detected in: * "/usr/bin" e.g. bash and python * "site-packages/" e.g. numpy * "pybind11.h" * Currently having an issue with leaking enum bindings * pybind/pybind11#3865 Signed-off-by: Jared Duffey <jared.duffey@bluequartz.net>
* Fixed issue with catch2 test discovery for asan CI * Catch2 runs the test executable in order to discover the tests but the executable couldn't find the clang asan library in the build step * Added leak suppressions file for LeakSanitizer * Ignore leaks detected in: * "/usr/bin" e.g. bash and python * "site-packages/" e.g. numpy * "pybind11.h" * Currently having an issue with leaking enum bindings * pybind/pybind11#3865 Signed-off-by: Jared Duffey <jared.duffey@bluequartz.net>
Required prerequisites
Problem description
Dear team, Asan reports possible memory leaks when wrapping enum from C++ (using
py::enum_
, precisely when a call to thevalue
method is done.Wrapping struct (using
py::class_
) seems to raise no leaks.Config : python3.8 / gcc8.3, pybind v2.9.2
May be related to #3228
Reproducible example code
The text was updated successfully, but these errors were encountered: