Propagate the error from the generalize request free callback to the user

[bosilca]

Make sure to reset the generalized request to guarantee not to call the free callback a second time.

Fixes #11681.

[jsquyres]

The behavior in the case of the user's function returning non-SUCCESS is a little odd:

1. The back-end object is not OBJ_RELEASE'd
2. The user's free function pointer is NULL'ed out

Meaning: if the user calls the freeing function a 2nd time (e.g., MPI_REQUEST_FREE):

1. The user's free function is NULL, so it won't be invoked again
2. The back-end object is OBJ_RELEASE'd

Is that intended?

[bosilca]

It is what makes sense to me. I assume that calling a second time the free function would generate the same outcome as the first call (aka. returning an error) and this will result in the resource never being released. With the approach implemented here, the second call to free will call directly into our object management (bypassing the user function) and will release the OMPI objects. In same time the user request will be set to MPI_REQUEST_NULL which means the user will not have any legitimate way to fiddle with the request again.

[jsquyres]

@bosilca Gotcha. I think that this is new and interesting behavior -- and I think it's valid behavior for an MPI implementation (i.e., call REQUEST_FREE more than once on the same request when an error occurs). I guess the app usage would need to be something like:

    // Or the equivalent in sessions
    MPI_Comm_set_errhandler(MPI_COMM_SELF, MPI_ERRORS_RETURN);

    int err = MPI_Request_free(&req);
    if (MPI_SUCCESS != err && req_is_generalized_request) {
        // Try again, because we might be in the case where 
        // the user-defined free function failed
        err = MPI_Request_free(&req);
    }
    if (MPI_SUCCESS != err) {
        // handle error
    }

At a minimum, we'd need to document this in the MPI_REQUEST_FREE man page.

This would set a new precedent for how to handle errors; are you thinking that this is a strategic direction in which Open MPI should go (w.r.t. handling errors)?

[bosilca]

@jsquyres the code is correct for MPI_Request_free, but the same techniques will need to be used for any completion function using generalized requests. Basically:

    // Or the equivalent in sessions
    MPI_Comm_set_errhandler(MPI_COMM_SELF, MPI_ERRORS_RETURN);

    int err = MPI_Wait(&req);
    if (MPI_SUCCESS != err && req_is_generalized_request) {
        // Try again, because we might be in the case where 
        // the user-defined free function failed
        err = MPI_Request_free(&req);
    }
    if (MPI_SUCCESS != err) {
        // handle error
    }

It does set a precedent in the sense that for generalized requests it gives us a way to release OMPI resources, something we are totally lacking today.

👍 for the documentation.

[jsquyres]

the code is correct for MPI_Request_free, but the same techniques will need to be used for any completion function using generalized requests.

Ok. My example was calling MPI_Request_free(&req) followed by MPI_Request_free(&req) inside the if block.

Your example called MPI_Wait(&req) followed by MPI_Request_free(&req) inside the if block.

Did you mean to call MPI_Wait(&req) inside the if block? Or are you saying that the 2nd call is always MPI_Request_free(&req) if the first completion function fails and it's a generalized request (i.e., inside the if block)?

[bosilca]

Yes, the 2nd call should always be a call to MPI_Request_free if any completion function fails and we are dealing with a generalized request.

[jsquyres]

Yes, the 2nd call should always be a call to MPI_Request_free if any completion function fails and we are dealing with a generalized request.

Should we return a specific error code to indicate that the reason the 1st completion function failed was because the user's free function failed? E.g., We could fail the 1st completion function for a different reason, and it may not be appropriate to call MPI_REQUEST_FREE. Perhaps something like this:

// Or the equivalent in sessions
MPI_Comm_set_errhandler(MPI_COMM_SELF, MPI_ERRORS_RETURN);

int err = MPI_Wait(&req);
if (MPIX_GREQUEST_USER_FREE_FUNC_FAILED == err) {
    // Try again, because we **are** in the case where 
    // the user-defined free function failed
    err = MPI_Request_free(&req);
}
if (MPI_SUCCESS != err) {
    // handle error
}

[bosilca]

I am sure that would not be compliant with the current MPI standard. Read 13.2 explanation for free_fn to see the extremely complicated and unfortunately well-defined requirements and interactions between query_fn and free_fn.

[jsquyres]

I am sure that would not be compliant with the current MPI standard. Read 13.2 explanation for free_fn to see the extremely complicated and unfortunately well-defined requirements and interactions between query_fn and free_fn.

I'm not disagreeing there -- I'm just wondering if we should return a specific error code so that users can tell that this specific error case is exactly what happened, and that they therefore should call MPI_REQUEST_FREE to actually free the resources.

That being said, if what this PR is doing is:

1. Telling the user some error occurred in the completion function
2. Not releasing the resources, and therefore not setting the user's handle to MPI_REQUEST_NULL (thereby telling the user to call MPI_REQUEST_FREE)

Is there a reason we don't just tell the user that some error occurred in the completion function, and also release the resources? I.e., why force the 2nd step? Specifically, instead of:

    if (OMPI_SUCCESS == rc ) {
        OBJ_RELEASE(*req);
        *req = MPI_REQUEST_NULL;
    } else {
        /* Make sure we will not be calling the grequest free function
         * a second time when we release the request.
         */
        greq->greq_free.c_free = NULL;
    }
    return rc;

have this:

    OBJ_RELEASE(*req);
    *req = MPI_REQUEST_NULL;
    return rc;

Put differently: is there something to be gained by forcing the user to call MPI_REQUEST_FREE?

[bosilca]

• once an error was raised by one of the user defined callbacks we are bound to return it either from a completion function or from the free. This means we have no opportunity to return something else to pinpoint the user to a secondary path.
• we cannot release the request in this function because, and here things are getting complicated, we might not have yet called the MPI_GREQUEST_COMPLETE so the generalized request object is still valid (according to the Advice to users on MPI 4.1 13.2).

[jsquyres]

@bosilca I did a little testing; I'm not sure this patch is right. I first tried to find out what MPICH does:

1. Lisandro's test program in Error propagation in free_fn callback for generalized requests #11681 correctly aborts because the user error function value is examined.
2. However, MPICH doesn't seem to handle the case where we set MPI_ERRORS_RETURN on MPI_COMM_SELF for generalized requests, so I can't tell what happens in MPICH if they don't abort.
3. I modified/extended Lisandro's test program:

#include <stdio.h>
#include <mpi.h>

static int query_fn  (void *ctx, MPI_Status *s) { return MPI_SUCCESS; }
static int free_fn   (void *ctx) { return MPI_ERR_OTHER; }  // <-- RETURN WITH FAILURE !!!                                          
static int cancel_fn (void *ctx, int c) { return MPI_SUCCESS;   }

static void test1(void)
{
    int ret;
    MPI_Status status;
    MPI_Request request;

    MPI_Grequest_start(query_fn, free_fn, cancel_fn, NULL, &request);
    MPI_Grequest_complete(request);

    ret = MPI_Wait(&request, &status);
    printf("Test 1: ret=%d, request==REQUEST_NULL: %d\n",
           ret, request == MPI_REQUEST_NULL);
}

static void test2(void)
{
    int ret;
    MPI_Request request, req_copy;

    MPI_Grequest_start(query_fn, free_fn, cancel_fn, NULL, &request);
    req_copy = request;

    ret = MPI_Request_free(&request);
    printf("Test 2: MPI_Request_free: ret=%d, request==REQUEST_NULL: %d\n",
           ret, request == MPI_REQUEST_NULL);
    ret = MPI_Grequest_complete(req_copy);
    printf("Test 2: MPI_Grequest_complete: ret=%d\n",
           ret);
}

static void test3(void)
{
    int ret;
    MPI_Status status;
    MPI_Request request, req_copy;

    MPI_Grequest_start(query_fn, free_fn, cancel_fn, NULL, &request);
    req_copy = request;

    ret = MPI_Grequest_complete(request);
    printf("Test 3: MPI_Grequest_complete: ret=%d\n",
           ret);
    ret = MPI_Request_free(&request);
    printf("Test 3: MPI_Request_free: ret=%d, request==REQUEST_NULL: %d\n",
           ret, request == MPI_REQUEST_NULL);

    ret = MPI_Wait(&request, &status);
    printf("Test 3: MPI_Wait: ret=%d, request==REQUEST_NULL: %d\n",
           ret, request == MPI_REQUEST_NULL);
}

int main(int argc, char *argv[])
{
    MPI_Init(&argc, &argv);
    MPI_Comm_set_errhandler(MPI_COMM_SELF, MPI_ERRORS_RETURN);

    test1();
    test2();
    test3();

    MPI_Finalize();
    return 0;
}

Here's the output I get:

Test 1: ret=16, request==REQUEST_NULL: 1
Test 2: MPI_Request_free: ret=16, request==REQUEST_NULL: 0
Test 2: MPI_Grequest_complete: ret=0
Test 3: MPI_Grequest_complete: ret=0
Test 3: MPI_Request_free: ret=16, request==REQUEST_NULL: 0
Test 3: MPI_Wait: ret=16, request==REQUEST_NULL: 1

In the first test, we do get MPI_REQUEST_NULL back -- there's no way to call MPI_Request_free() on the request.

In test 2, I'm not sure what it means that it apparently called the generalized free function before I called MPI_Grequest_complete(). That shouldn't happen, right?

In test 3 is different than test 2 only because it calls Grequest_complete before Request_free. But we still see that MPI_Request_free() still does not set request to MPI_REQUEST_NULL.

Hence, we're seeing different behavior here:

• MPI_Wait() is returning an error, but setting the request to MPI_REQUEST_NULL.
• MPI_Request_free() is also returning an error, but it is not setting the request to MPI_REQUEST_NULL.

[bosilca]

Did some updates, but I'm still puzzled by the intent of the generalized requests.

In any case, @jsquyres I don't think your test3 is legal. The standard clearly states that once MPI_Request_free has been called on a request, no completion function shall be called. In addition to this, the generalized request section (v4 13.2), clearly states:

The free_fn callback is also invoked for generalized requests that are freed by a call to MPI_REQUEST_FREE (no call to MPI_{WAIT|TEST}{ANY|SOME|ALL} will occur for such a request). In this case, the callback function will be called either in the MPI call MPI_REQUEST_FREE(request), or in the MPI call MPI_GREQUEST_COMPLETE(request), whichever happens last, i.e., in this case the actual freeing code is executed as soon as both calls MPI_REQUEST_FREE and MPI_GREQUEST_COMPLETE have occurred.

[jsquyres]

@bosilca and I chatted about this on the phone. We're pretty convinced that this latest commit is correct. @bosilca is going to add a comment in grequest.c to explain a subtlety in the error path of ompi_grequest_free(), which inadvertently led to the lengthy discussion about error handling.

The whole conversation prior to this about the user needing to call MPI_REQUEST_FREE after an error occurs is now moot (i.e., it is not necessary). So there's really no new handling of errors here, no new precedent, ...etc. It's just a subtlety in how the base request is actually freed in the error path. @bosilca's comment will explain.

I'll approve when the new commit gets here with the comment.

[gpaulsen]

bot:ibm:retest

[dalcinl]

@jsquyres @bosilca Any chance that we can get this one in for next release v5.0.0?

[jsquyres]

Hey @bosilca -- can you add the comment as was described in #11683 (review)? Then we can get this PR merged.

[dalcinl]

@jsquyres @bosilca Any chance that we can get this one in for next release v5.0.2?

[jsquyres]

Sadly, neither @bosilca nor I remember what the subtle issue is/was 😦 and we kinda need this PR now. Sooo... let's merge. @bosilca said in Slack:

that’s why I was deferring it in the first place, I don’t recall what I was expected to add to it
just merge it, if the issue ever comes back I’ll figure it out

So there's @bosilca's promise to figure it out if we need it again. 😉