Merge pull request #11683 from bosilca/topic/fix_grequest_free

Propagate the error from the generalize request free callback to the user
open-mpi · Jan 19, 2024 · e534611 · e534611
2 parents 6ab6775 + ac3647e
commit e534611
Show file tree

Hide file tree

Showing 2 changed files with 47 additions and 20 deletions.
diff --git a/ompi/request/grequest.c b/ompi/request/grequest.c
@@ -24,6 +24,32 @@
 #include "ompi/request/grequest.h"
 #include "ompi/mpi/fortran/base/fint_2_int.h"
 
+/**
+ * Internal function to specialize the call to the user provided free_fn
+ * for generalized requests.
+ * @return The return value of the user specified callback or MPI_SUCCESS.
+ */
+static inline int ompi_grequest_internal_free(ompi_grequest_t* greq)
+{
+    int rc = MPI_SUCCESS;
+    if (NULL != greq->greq_free.c_free) {
+        /* We were already putting query_fn()'s return value into
+         * status.MPI_ERROR but for MPI_{Wait,Test}*.  If there's a
+         * free callback to invoke, the standard says to use the
+         * return value from free_fn() callback, too.
+         */
+        if (greq->greq_funcs_are_c) {
+            greq->greq_base.req_status.MPI_ERROR =
+                greq->greq_free.c_free(greq->greq_state);
+        } else {
+            MPI_Fint ierr;
+            greq->greq_free.f_free((MPI_Aint*)greq->greq_state, &ierr);
+            greq->greq_base.req_status.MPI_ERROR = OMPI_FINT_2_INT(ierr);
+        }
+        rc = greq->greq_base.req_status.MPI_ERROR;
+    }
+    return rc;
+ }
 
 /*
  * See the comment in the grequest destructor for the weird semantics
@@ -37,9 +63,21 @@
  */
 static int ompi_grequest_free(ompi_request_t** req)
 {
-    OBJ_RELEASE(*req);
-    *req = MPI_REQUEST_NULL;
-    return OMPI_SUCCESS;
+    ompi_grequest_t* greq = (ompi_grequest_t*)*req;
+    int rc = OMPI_SUCCESS;
+
+    if( greq->greq_user_freed ) {
+        return OMPI_ERR_OUT_OF_RESOURCE;
+    }
+    greq->greq_user_freed = true;
+    if( REQUEST_COMPLETE(*req) ) {
+        rc = ompi_grequest_internal_free(greq);
+    }
+    if (OMPI_SUCCESS == rc ) {
+        OBJ_RELEASE(*req);
+        *req = MPI_REQUEST_NULL;
+    }
+    return rc;
 }
 
 static int ompi_grequest_cancel(ompi_request_t* req, int flag)
@@ -72,6 +110,7 @@ static void ompi_grequest_construct(ompi_grequest_t* greq)
        override this value if the gen request was created from
        Fortran */
     greq->greq_funcs_are_c = true;
+    greq->greq_user_freed = false;
 }
 
 /*
@@ -122,23 +161,6 @@ static void ompi_grequest_construct(ompi_grequest_t* greq)
  */
 static void ompi_grequest_destruct(ompi_grequest_t* greq)
 {
-    if (greq->greq_free.c_free != NULL) {
-        /* We were already putting query_fn()'s return value into
-         * status.MPI_ERROR but for MPI_{Wait,Test}*.  If there's a
-         * free callback to invoke, the standard says to use the
-         * return value from free_fn() callback, too.
-         */
-        if (greq->greq_funcs_are_c) {
-            greq->greq_base.req_status.MPI_ERROR =
-                greq->greq_free.c_free(greq->greq_state);
-        } else {
-            MPI_Fint ierr;
-            greq->greq_free.f_free((MPI_Aint*)greq->greq_state, &ierr);
-            greq->greq_base.req_status.MPI_ERROR =
-                OMPI_FINT_2_INT(ierr);
-        }
-    }
-
     OMPI_REQUEST_FINI(&greq->greq_base);
 }
 
@@ -188,9 +210,13 @@ int ompi_grequest_start(
  */
 int ompi_grequest_complete(ompi_request_t *req)
 {
+    ompi_grequest_t* greq = (ompi_grequest_t*)req;
     int rc;
 
     rc = ompi_request_complete(req, true);
+    if( greq->greq_user_freed ) {
+        rc = ompi_grequest_internal_free(greq);
+    }
     OBJ_RELEASE(req);
     return rc;
 }

diff --git a/ompi/request/grequest.h b/ompi/request/grequest.h
@@ -104,6 +104,7 @@ struct ompi_grequest_t {
 #endif
     void *greq_state;
     bool greq_funcs_are_c;
+    bool greq_user_freed;
 };
 /**
  * Convenience typedef