Python: reacquire GIL for deallocation operations (#1878)

* Delete stray inc_ref

* Reaquire the GIL for releasing references to Python buffers

* Use std::optional for simplified explicit destruction

* Make Iterator GIL free

* Add this also for other flushing methods

Author: franzpoeschel

src/binding/python/Attributable.cpp

@@ -518,11 +518,17 @@ void init_Attributable(py::module &m)
             })
         .def(
             "series_flush",
-            py::overload_cast<std::string>(&Attributable::seriesFlush),
+            [](Attributable &attr, std::string const &backend_config) {
+                py::gil_scoped_release release_gil;
+                attr.seriesFlush(backend_config);
+            },
             py::arg("backend_config") = "{}")
         .def(
             "iteration_flush",
-            py::overload_cast<std::string>(&Attributable::iterationFlush),
+            [](Attributable &attr, std::string const &backend_config) {
+                py::gil_scoped_release release_gil;
+                attr.iterationFlush(backend_config);
+            },
             py::arg("backend_config") = "{}")
 
         .def_property_readonly(

src/binding/python/Iteration.cpp

@@ -82,11 +82,10 @@ void init_Iteration(py::module &m)
             })
         .def(
             "close",
-            /*
-             * Cannot release the GIL here since Python buffers might be
-             * accessed in deferred tasks
-             */
-            &Iteration::close,
+            [](Iteration &iteration, bool flush) {
+                py::gil_scoped_release release_gil;
+                iteration.close(flush);
+            },
             py::arg("flush") = true)
 
         // TODO remove in future versions (deprecated)

src/binding/python/RecordComponent.cpp

@@ -20,6 +20,7 @@
  */
 #include <limits>
 #include <pybind11/detail/common.h>
+#include <pybind11/gil.h>
 #include <pybind11/numpy.h>
 #include <pybind11/pybind11.h>
 #include <pybind11/pytypes.h>
@@ -331,8 +332,11 @@ struct StoreChunkFromPythonArray
         // note: this does not yet prevent the user, as in C++, to build
         // a race condition by manipulating the data that was passed
         std::shared_ptr<T> shared(
-            (T *)data, [owning_handle = a.cast<py::object>()](T *) {
-                // no-op
+            (T *)data,
+            [owning_handle =
+                 std::make_optional(a.cast<py::object>())](T *) mutable {
+                py::gil_scoped_acquire need_the_gil_for_this;
+                owning_handle.reset();
             });
         r.storeChunk(std::move(shared), offset, extent);
     }
@@ -354,8 +358,11 @@ struct LoadChunkIntoPythonArray
         // note: this does not yet prevent the user, as in C++, to build
         // a race condition by manipulating the data that was passed
         std::shared_ptr<T> shared(
-            (T *)data, [owning_handle = a.cast<py::object>()](T *) {
-                // no-op
+            (T *)data,
+            [owning_handle =
+                 std::make_optional(a.cast<py::object>())](T *) mutable {
+                py::gil_scoped_acquire need_the_gil_for_this;
+                owning_handle.reset();
             });
         r.loadChunk(std::move(shared), offset, extent);
     }
@@ -378,8 +385,11 @@ struct LoadChunkIntoPythonBuffer
         // note: this does not yet prevent the user, as in C++, to build
         // a race condition by manipulating the data that was passed
         std::shared_ptr<T> shared(
-            (T *)data, [owning_handle = buffer.cast<py::object>()](T *) {
-                // no-op
+            (T *)data,
+            [owning_handle =
+                 std::make_optional(buffer.cast<py::object>())](T *) mutable {
+                py::gil_scoped_acquire need_the_gil_for_this;
+                owning_handle.reset();
             });
         r.loadChunk(std::move(shared), offset, extent);
     }

src/binding/python/Series.cpp

@@ -35,6 +35,7 @@
 
 #include <optional>
 #include <pybind11/attr.h>
+#include <pybind11/gil.h>
 #include <stdexcept>
 #include <tuple>
 
@@ -283,22 +284,8 @@ not possible once it has been closed.
             .def(
                 "__getitem__",
                 [](Snapshots &s, Series::IterationIndex_t key) {
-                    switch (s.snapshotWorkflow())
-                    {
-                    case openPMD::SnapshotWorkflow::RandomAccess:
-                        return s[key];
-                    case openPMD::SnapshotWorkflow::Synchronous:
-                        auto lastIteration = s.currentIteration();
-                        if (lastIteration.has_value() &&
-                            lastIteration.value()->first != key)
-                        {
-                            // this must happen under the GIL
-                            lastIteration.value()->second.close();
-                        }
-                        py::gil_scoped_release release;
-                        return s[key];
-                    }
-                    throw std::runtime_error("Unreachable");
+                    py::gil_scoped_release release;
+                    return s[key];
                 },
                 // copy + keepalive
                 py::return_value_policy::copy,
@@ -336,10 +323,6 @@ not possible once it has been closed.
                  */
                 if (!iterator.first_iteration)
                 {
-                    if (!(*iterator).closed())
-                    {
-                        (*iterator).close();
-                    }
                     py::gil_scoped_release release;
                     ++iterator;
                 }
@@ -482,7 +465,13 @@ this method.
             &Series::iterationFormat,
             &Series::setIterationFormat)
         .def_property("name", &Series::name, &Series::setName)
-        .def("flush", &Series::flush, py::arg("backend_config") = "{}")
+        .def(
+            "flush",
+            [](Series &s, std::string const &backend_config) {
+                py::gil_scoped_release release_gil;
+                s.flush(backend_config);
+            },
+            py::arg("backend_config") = "{}")
 
         .def_property_readonly(
             "backend", static_cast<std::string (Series::*)()>(&Series::backend))