Skip to content

UCP/EP/FLUSH: fix too early completion for failed EP - #5952

Merged
yosefe merged 1 commit into
openucx:masterfrom
evgeny-leksikov:ucp_ep_flush_internal_failed
Nov 26, 2020
Merged

yosefe merged 1 commit into
openucx:masterfrom
evgeny-leksikov:ucp_ep_flush_internal_failed

Conversation

@evgeny-leksikov

Copy link
Copy Markdown
Contributor

What

fix too early completion for failed EP

Why ?

in case if scheduled err handler was not be invoked yet, the EP can be closed with requests in pending
reproducer is implemented in #5931 but that PR has other dependency

}

ASSERT_UCS_OK(UCS_PTR_STATUS(req));
/* close request can be completed with any status depends on peer state */

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok for now, in next PR by @Artemy-Mellanox close(FORCE) will always return NULL (success)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but close(flush) can be completed with err too

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, close (flush) can complete with err
can u pls open another PR with doc clarification on this:
e.g close(flush) operation can return failure if the remote side disconnected at same time, but nevertheless all resources associated with the endpoint will be destroyed.
close(force) will always succeed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO this way we add extra commitment without any benefit.
maybe don't differentiate flush/force return status and just mention that "close request can be completed with error if the remote side disconnected at same time but nevertheless all resources associated with the endpoint will be destroyed"?

@yosefe

yosefe commented Nov 25, 2020

Copy link
Copy Markdown
Member

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@evgeny-leksikov
evgeny-leksikov force-pushed the ucp_ep_flush_internal_failed branch from 6960ee4 to a89ce65 Compare November 26, 2020 07:45
@evgeny-leksikov

Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@evgeny-leksikov

Copy link
Copy Markdown
Contributor Author

failure was #3897

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants