From d6474ed7d28c03ef79acd2e87ca429072727df86 Mon Sep 17 00:00:00 2001
From: Tailing Yuan <yuantailing@gmail.com>
Date: Tue, 31 Aug 2021 09:48:33 +0800
Subject: [PATCH] fix: memory leak in cpp_function (#3228) (#3229)

* fix: memory leak in cpp_function (#3228)

* add a test case to check objects are deconstructed in cpp_function

* update the test case about cpp_function

* fix the test case about cpp_function: remove "noexcept"

* Actually calling func. CHECK(stat.alive() == 2); Manually verified that the new tests fails without the change in pybind11.h

* Moving new test to test_callbacks.cpp,py, with small enhancements.

* Removing new test from test_interpreter.cpp (after it was moved to test_callbacks.cpp,py). This restores test_interpreter.cpp to the current state on master.

* Using py::detail::silence_unused_warnings(py_func); to make the intent clear.

Co-authored-by: Ralf W. Grosse-Kunstleve <rwgk@google.com>
---
 include/pybind11/pybind11.h |  2 +-
 tests/test_callbacks.cpp    | 43 +++++++++++++++++++++++++++++++++++--
 tests/test_callbacks.py     |  7 +++++-
 3 files changed, 48 insertions(+), 4 deletions(-)

diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h
index 89f9cbd9b..990c21304 100644
--- a/include/pybind11/pybind11.h
+++ b/include/pybind11/pybind11.h
@@ -175,7 +175,7 @@ protected:
 #endif
             // UB without std::launder, but without breaking ABI and/or
             // a significant refactoring it's "impossible" to solve.
-            if (!std::is_trivially_destructible<Func>::value)
+            if (!std::is_trivially_destructible<capture>::value)
                 rec->free_data = [](function_record *r) {
                     auto data = PYBIND11_STD_LAUNDER((capture *) &r->data);
                     (void) data;
diff --git a/tests/test_callbacks.cpp b/tests/test_callbacks.cpp
index a50771038..58688b6e8 100644
--- a/tests/test_callbacks.cpp
+++ b/tests/test_callbacks.cpp
@@ -81,16 +81,55 @@ TEST_SUBMODULE(callbacks, m) {
     };
     // Export the payload constructor statistics for testing purposes:
     m.def("payload_cstats", &ConstructorStats::get<Payload>);
-    /* Test cleanup of lambda closure */
-    m.def("test_cleanup", []() -> std::function<void()> {
+    m.def("test_lambda_closure_cleanup", []() -> std::function<void()> {
         Payload p;
 
+        // In this situation, `Func` in the implementation of
+        // `cpp_function::initialize` is NOT trivially destructible.
         return [p]() {
             /* p should be cleaned up when the returned function is garbage collected */
             (void) p;
         };
     });
 
+    class CppCallable {
+    public:
+        CppCallable() { track_default_created(this); }
+        ~CppCallable() { track_destroyed(this); }
+        CppCallable(const CppCallable &) { track_copy_created(this); }
+        CppCallable(CppCallable &&) noexcept { track_move_created(this); }
+        void operator()() {}
+    };
+
+    m.def("test_cpp_callable_cleanup", []() {
+        // Related issue: https://github.com/pybind/pybind11/issues/3228
+        // Related PR: https://github.com/pybind/pybind11/pull/3229
+        py::list alive_counts;
+        ConstructorStats &stat = ConstructorStats::get<CppCallable>();
+        alive_counts.append(stat.alive());
+        {
+            CppCallable cpp_callable;
+            alive_counts.append(stat.alive());
+            {
+                // In this situation, `Func` in the implementation of
+                // `cpp_function::initialize` IS trivially destructible,
+                // only `capture` is not.
+                py::cpp_function py_func(cpp_callable);
+                py::detail::silence_unused_warnings(py_func);
+                alive_counts.append(stat.alive());
+            }
+            alive_counts.append(stat.alive());
+            {
+                py::cpp_function py_func(std::move(cpp_callable));
+                py::detail::silence_unused_warnings(py_func);
+                alive_counts.append(stat.alive());
+            }
+            alive_counts.append(stat.alive());
+        }
+        alive_counts.append(stat.alive());
+        return alive_counts;
+    });
+
     // test_cpp_function_roundtrip
     /* Test if passing a function pointer from C++ -> Python -> C++ yields the original pointer */
     m.def("dummy_function", &dummy_function);
diff --git a/tests/test_callbacks.py b/tests/test_callbacks.py
index 9dc272a2d..edbb1890c 100644
--- a/tests/test_callbacks.py
+++ b/tests/test_callbacks.py
@@ -79,13 +79,18 @@ def test_keyword_args_and_generalized_unpacking():
 
 
 def test_lambda_closure_cleanup():
-    m.test_cleanup()
+    m.test_lambda_closure_cleanup()
     cstats = m.payload_cstats()
     assert cstats.alive() == 0
     assert cstats.copy_constructions == 1
     assert cstats.move_constructions >= 1
 
 
+def test_cpp_callable_cleanup():
+    alive_counts = m.test_cpp_callable_cleanup()
+    assert alive_counts == [0, 1, 2, 1, 2, 1, 0]
+
+
 def test_cpp_function_roundtrip():
     """Test if passing a function pointer from C++ -> Python -> C++ yields the original pointer"""