ompi/oshmem/spml/ucx: Fix cleanup when disconnected: avoid double free

[iyastreb]

Fixes #12249

Problem:
Crash occurs when running hello_oshmem_c.c example, which happens due to double free in oshmem_shmem_finalize function.

Root cause analysis:
oshmem_shmem_finalize is called twice, first time it's called explicitly from the app, and the second time it's called from shmem_onexit handler. It's perfectly fine to call this function multiple times, as it's protected by oshmem_shmem_initialized variable, which prevents double free on second invocation.
The problem is that this flag is not being reset, due to the error in _shmem_finalize:

oshmem_shmem_finalize
    ret = _shmem_finalize();
        mca_spml_base_finalize
            mca_spml_ucx_component_fini
                ret = opal_common_ucx_mca_pmix_fence_nb <<< returns PMIX_ERR_UNREACH when disconnected
                if (ret != OPAL_SUCCESS)
                    return ret;
            
    if (OSHMEM_SUCCESS == ret) {
        oshmem_shmem_initialized = false;
    }

When we are disconnected, then opal_common_ucx_mca_pmix_fence_nb returns PMIX_ERR_UNREACH error, and we interrupt the cleanup process in the middle. Then subsequent cleanup call tries to free the freed memory (requests) => segfault.

Solution
Allow clean up to proceed when we are in disconnected state. If we are not connected to the server, that's ok, we can probably successfully release the resources. IMO being connected to the server should not be prerequisite for destroy.

[github-actions]

Hello! The Git Commit Checker CI bot found a few problems with this PR:

3372d13: ompi/oshmem/spml/ucx: Fix clean up when disconnect...

• check_signed_off: does not contain a valid Signed-off-by line

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

[brminich]

@tvegas1, can you pls review?

[tvegas1]

would it also make sense to set fenced = 1 also for default case, maybe with a log?

[iyastreb]

No, I don't think it makes sense.
fenced is just a local variable on stack used below to safely clean up the resources.
If we return an error, no need to set it

[tvegas1]

i meant, setting it to 1 and continuing with cleanup anyways, wdyt?

[iyastreb]

Ok, I see your point now.
Basically it means that if we don't manage to contact the server, we just go on with clean up.
To me it looks reasonable, because if we return an error here and interrupt the cleanup, then we remain in semi-destroyed state, and subsequent cleanup call will crash anyway.

On the other side, existing code in _shmem_finalize follows the pattern: check status of each cleanup stage and return an error:
https://github.com/open-mpi/ompi/blob/main/oshmem/runtime/oshmem_shmem_finalize.c#L90

[tvegas1]

ok we could keep it like this then

[brminich]

pls note that branch is out of date, so needs to be synced

[github-actions]

Hello! The Git Commit Checker CI bot found a few problems with this PR:

0f16fbe: ompi/oshmem/spml/ucx: Fix cleanup when disconnecte...

• check_signed_off: does not contain a valid Signed-off-by line

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

[yosefe]

print a warning if fence fails

[iyastreb]

done

[yosefe]

is there a function to also print string name of the error code?

[rhc54]

PMIx_Error_string(ret)

[iyastreb]

ok, changed

[jsquyres]

@iyastreb Please don't add merge commits into PR's; if you want the latest from main, please rebase.

[iyastreb]

@iyastreb Please don't add merge commits into PR's; if you want the latest from main, please rebase.

@jsquyres Ok, sure.
In this case I just noticed that my PR is marked as out-of-date, because of some recent changes in main.
So I just pressed the button "Update branch" in GH webUI, which resulted in this merge

[jsquyres]

@jsquyres Ok, sure. In this case I just noticed that my PR is marked as out-of-date, because of some recent changes in main. So I just pressed the button "Update branch" in GH webUI, which resulted in this merge

Fair enough. I wish there was a way we could disable Github from showing that button.