Merge pull request #2082 from stack-of-tasks/topic/urdf_memory_leak

Topic/urdf memory leak

bindings/python/parsers/urdf/geometry.cpp

@@ -139,10 +139,20 @@ namespace pinocchio
       template <class ArgumentPackage>
       static PyObject* postcall(ArgumentPackage const& args_, PyObject* result)
       {
+        // If owner_arg exists, we run bp::return_internal_reference postcall
+        // result lifetime will be tied to the owner_arg lifetime
         PyObject* patient = bp::detail::get_prev<owner_arg>::execute(args_, result);
         if (patient != Py_None)
           return bp::return_internal_reference<owner_arg>::postcall(args_, result);
-        return result;
+        // If owner_arg doesn't exist, then Python will have to manage the result lifetime
+        bp::extract<GeometryModel*> geom_model_extract(result);
+        if (geom_model_extract.check())
+        {
+          return bp::to_python_indirect<GeometryModel, bp::detail::make_owning_holder>()(geom_model_extract());
+        }
+        // If returned value is not a GeometryModel*, then raise an error
+        PyErr_SetString(PyExc_RuntimeError, "pinocchio::python::return_value_policy only works on GeometryModel* data type");
+        return 0;
       }
     };
 

unittest/python/CMakeLists.txt

@@ -1,7 +1,46 @@
 #
-# Copyright (c) 2015-2021 CNRS INRIA
+# Copyright (c) 2015-2023 CNRS INRIA
 #
 
+macro(ADD_PYTHON_MEMORYCHECK_UNIT_TEST NAME SOURCE)
+  set(TEST_FILE_NAME memorycheck_unit_test_${NAME}.cmake)
+  set(PYTHON_TEST_SCRIPT "${PROJECT_SOURCE_DIR}/${SOURCE}")
+  configure_file(memorycheck_unit_test.cmake.in ${TEST_FILE_NAME} @ONLY)
+
+  add_test(NAME ${NAME}
+    COMMAND ${CMAKE_COMMAND} -P ${TEST_FILE_NAME})
+
+  set(MODULES "${ARGN}") # ARGN is not a variable
+  foreach(MODULE_PATH IN LISTS MODULES)
+    list(APPEND PYTHONPATH "${CMAKE_BINARY_DIR}/${MODULE_PATH}")
+  endforeach()
+
+  if(DEFINED ENV{PYTHONPATH})
+    list(APPEND PYTHONPATH "$ENV{PYTHONPATH}")
+  endif()
+
+  # get path separator to join those paths
+  execute_process(
+    COMMAND "${PYTHON_EXECUTABLE}" "-c" "import os; print(os.pathsep)"
+    OUTPUT_VARIABLE PATHSEP
+    OUTPUT_STRIP_TRAILING_WHITESPACE)
+
+  list(REMOVE_DUPLICATES PYTHONPATH)
+  if(WIN32)
+    # ensure that severals paths stay together as ENV variable PYTHONPATH when
+    # passed to python test via PROPERTIES
+    string(REPLACE ";" "\;" PYTHONPATH_STR "${PYTHONPATH}")
+  else(WIN32)
+    string(REPLACE ";" "${PATHSEP}" PYTHONPATH_STR "${PYTHONPATH}")
+  endif(WIN32)
+  set(ENV_VARIABLES "PYTHONPATH=${PYTHONPATH_STR}")
+  if(APPLE)
+    list(APPEND ENV_VARIABLES "LD_LIBRARY_PATH=$ENV{LD_LIBRARY_PATH}")
+    list(APPEND ENV_VARIABLES "DYLD_LIBRARY_PATH=$ENV{DYLD_LIBRARY_PATH}")
+  endif(APPLE)
+  set_tests_properties(${NAME} PROPERTIES ENVIRONMENT "${ENV_VARIABLES}")
+endmacro()
+
 SET(${PROJECT_NAME}_PYTHON_TESTS
   bindings
 
@@ -73,10 +112,24 @@ IF(urdfdom_FOUND)
     )
 ENDIF(urdfdom_FOUND)
 
+SET(${PROJECT_NAME}_PYTHON_MEMORYCHECK_TESTS
+  bindings_build_geom_from_urdf_memorycheck
+  )
+
 FOREACH(TEST ${${PROJECT_NAME}_PYTHON_TESTS})
   ADD_PYTHON_UNIT_TEST("test-py-${TEST}" "unittest/python/${TEST}.py" "bindings/python")
 ENDFOREACH(TEST ${${PROJECT_NAME}_PYTHON_TESTS})
 
 MAKE_DIRECTORY("${CMAKE_CURRENT_BINARY_DIR}/serialization-data")
 
+IF(MEMORYCHECK_COMMAND AND MEMORYCHECK_COMMAND MATCHES ".*valgrind$")
+  FOREACH(TEST ${${PROJECT_NAME}_PYTHON_MEMORYCHECK_TESTS})
+    ADD_PYTHON_MEMORYCHECK_UNIT_TEST("test-py-memory-${TEST}"
+      "unittest/python/${TEST}.py"
+      "bindings/python")
+  ENDFOREACH()
+ELSE()
+  MESSAGE(STATUS "Valgrind not found, memory checks are disabled")
+ENDIF()
+
 ADD_SUBDIRECTORY(pybind11)

unittest/python/bindings_build_geom_from_urdf_memorycheck.py

@@ -0,0 +1,30 @@
+import os
+import unittest
+
+import pinocchio as pin
+
+
+@unittest.skipUnless(pin.WITH_URDFDOM, "Needs URDFDOM")
+class TestBuildGeomFromUrdfMemoryCheck(unittest.TestCase):
+    def setUp(self):
+        self.current_file = os.path.dirname(str(os.path.abspath(__file__)))
+        self.model_dir = os.path.abspath(
+            os.path.join(self.current_file, "../../models/example-robot-data/robots")
+        )
+        self.model_path = os.path.abspath(
+            os.path.join(self.model_dir, "ur_description/urdf/ur5_robot.urdf")
+        )
+
+    def test_load(self):
+        model = pin.buildModelFromUrdf(self.model_path)
+        for _ in range(2):
+            pin.buildGeomFromUrdf(
+                model,
+                self.model_path,
+                pin.COLLISION,
+                package_dirs=self.model_dir,
+            )
+
+
+if __name__ == "__main__":
+    unittest.main()

unittest/python/memorycheck_unit_test.cmake.in

@@ -0,0 +1,17 @@
+SET(PYTHON_EXECUTABLE @PYTHON_EXECUTABLE@)
+SET(MEMORYCHECK_COMMAND @MEMORYCHECK_COMMAND@)
+SET(PYTHON_TEST_SCRIPT @PYTHON_TEST_SCRIPT@)
+
+execute_process(COMMAND
+  ${MEMORYCHECK_COMMAND} -- ${PYTHON_EXECUTABLE} ${PYTHON_TEST_SCRIPT}
+  ERROR_VARIABLE MEMORYCHECK_OUTPUT)
+
+string(FIND "${MEMORYCHECK_OUTPUT}" "definitely lost: 0 bytes in 0 blocks" DEFINITELY_LOST)
+string(FIND "${MEMORYCHECK_OUTPUT}" "indirectly lost: 0 bytes in 0 blocks" INDIRECTLY_LOST)
+
+if(${DEFINITELY_LOST} GREATER -1 AND ${INDIRECTLY_LOST} GREATER -1)
+  message(STATUS "${PYTHON_TEST_SCRIPT} is not leaking memory")
+else()
+  message(FATAL_ERROR "Output: ${MEMORYCHECK_OUTPUT}\n"
+    "${PYTHON_TEST_SCRIPT} is leaking memory\n")
+endif()

unittest/python/utils.py

@@ -15,9 +15,9 @@ def test_se3ToXYZQUAT_XYZQUATToSe3(self):
         self.assertApprox(pin.SE3ToXYZQUAT(m).T, [1., 2., 3., sqrt(2) / 2, 0, 0, sqrt(2) / 2])
         self.assertApprox(pin.XYZQUATToSE3([1., 2., 3., sqrt(2) / 2, 0, 0, sqrt(2) / 2]), m)
         self.assertApprox(pin.XYZQUATToSE3(np.array([1., 2., 3., sqrt(2) / 2, 0, 0, sqrt(2) / 2])), m)
-        with self.assertRaisesRegex(ValueError, "Wrong size: .*"):
+        with self.assertRaises(ValueError):
             pin.XYZQUATToSE3([1., 2., 3.])
-        with self.assertRaisesRegex(ValueError, "Wrong size: .*"):
+        with self.assertRaises(ValueError):
             pin.XYZQUATToSE3(np.array([1., 2., 3., sqrt(2) / 2]))
 
     def test_isapprox(self):