From d910b74f55997444be3ba5f7776ac0bedda85507 Mon Sep 17 00:00:00 2001
From: Luc Grosheintz <luc.grosheintz@gmail.com>
Date: Thu, 21 Nov 2024 09:53:05 +0100
Subject: [PATCH] Modernize DECREF (MPI edition). (#3207)

This commit refactors a part of the NRN Python bindings to use `nanobind`
objects instead of `Py_DECREF`. The purpose is to simplify the DECREFing logic
on error paths; and the risk of leaking when exceptions are thrown.

As part of the refactoring, if needed, the scope of certain variables might be
reduced or a given a new name. Additionally, NULL pointers are replaced with
`nullptr`.

This commit doesn't intentionally change reference counts.
---
 src/nrnpython/nrnpy_p2h.cpp | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/src/nrnpython/nrnpy_p2h.cpp b/src/nrnpython/nrnpy_p2h.cpp
index 9267abdd2b..4cf0f5dba8 100644
--- a/src/nrnpython/nrnpy_p2h.cpp
+++ b/src/nrnpython/nrnpy_p2h.cpp
@@ -817,19 +817,19 @@ static Object* py_alltoall_type(int size, int type) {
 #if NRNMPI
     setpickle();
     int root;
-    PyObject* pdest = NULL;
+    nb::object pdest;
 
     if (type == 2) {
-        pdest = py_allgather(psrc.ptr());
+        pdest = nb::steal(py_allgather(psrc.ptr()));
     } else if (type != 1 && type != 5) {
         root = size;
         if (root < 0 || root >= np) {
             hoc_execerror("root rank must be >= 0 and < nhost", 0);
         }
         if (type == 3) {
-            pdest = py_gather(psrc.ptr(), root);
+            pdest = nb::steal(py_gather(psrc.ptr(), root));
         } else if (type == 4) {
-            pdest = py_broadcast(psrc.ptr(), root);
+            pdest = nb::steal(py_broadcast(psrc.ptr(), root));
         }
     } else {
         if (type == 5) {  // scatter
@@ -883,7 +883,7 @@ static Object* py_alltoall_type(int size, int type) {
             sdispl = mk_displ(scnt.data());
             rdispl = mk_displ(rcnt);
             if (size < 0) {
-                pdest = nb::make_tuple(sdispl[np], rdispl[np]).release().ptr();
+                pdest = nb::make_tuple(sdispl[np], rdispl[np]);
                 delete[] sdispl;
                 delete[] rcnt;
                 delete[] rdispl;
@@ -892,7 +892,7 @@ static Object* py_alltoall_type(int size, int type) {
                 nrnmpi_char_alltoallv(s.data(), scnt.data(), sdispl, r, rcnt, rdispl);
                 delete[] sdispl;
 
-                pdest = char2pylist(r, np, rcnt, rdispl);
+                pdest = nb::steal(char2pylist(r, np, rcnt, rdispl));
 
                 delete[] r;
                 delete[] rcnt;
@@ -915,19 +915,29 @@ static Object* py_alltoall_type(int size, int type) {
                 delete[] sdispl;
 
             if (rcnt[0]) {
-                nb::object po = unpickle(r);
-                pdest = po.release().ptr();
+                pdest = unpickle(r);
             } else {
-                pdest = Py_None;
-                Py_INCREF(pdest);
+                pdest = nb::none();
             }
 
             delete[] rcnt;
             assert(rdispl == NULL);
         }
     }
-    Object* ho = nrnpy_po2ho(pdest);
-    Py_XDECREF(pdest);
+
+    Object* ho = nrnpy_po2ho(pdest.ptr());
+
+    // The problem here is when `pdest` is a HOC object. In that case `ho` has `refcount == 2` but
+    // must be returned with refcount 0. The `ho` is contained in the `pdest` (which is why the HOC
+    // refcount is 2) and will be destroyed along with the `pdest` if its reference count is 0.
+    //
+    // Therefore, we must first DECREF `pdest` and then decrement `ho->refcount`. If we do
+    // it the other way around the `Py_DECREF` causes the Python object to be deallocated,
+    // as part of that deallocation method, the contained HOC object is dereferenced, and because
+    // it'll have a reference count of 0 it's also be deallocated.
+    pdest.dec_ref();
+    pdest.release();
+
     if (ho) {
         --ho->refcount;
     }