New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix error handling for generalized requests #12392
base: main
Are you sure you want to change the base?
Changes from all commits
283aa97
76c6b5a
ea5cb80
d73fb7a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -532,7 +532,7 @@ static inline int ompi_request_complete(ompi_request_t* request, bool with_signa | |
|
||
ompi_wait_sync_t *tmp_sync = (ompi_wait_sync_t *) OPAL_ATOMIC_SWAP_PTR(&request->req_complete, | ||
REQUEST_COMPLETED); | ||
if( REQUEST_PENDING != tmp_sync ) { | ||
if (REQUEST_PENDING != tmp_sync && REQUEST_COMPLETED != tmp_sync) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that a core premise of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
IMHO, it is probably easier to just allow multiple |
||
wait_sync_update(tmp_sync, 1, request->req_status.MPI_ERROR); | ||
} | ||
} else { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did
ompi_request_free(request)
already set this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the generic user-level
MPI_Request_free()
for all kinds of requests, not only generalized requests. I'm not so expert with the Open MPI implementation to 100% sureompi_request_free()
will set the status error field on errors. Without that guarantee, my use ofompi_errhandler_request_invoke()
may not work as I intended. Note thatompi_errhandler_request_invoke()
has the side-effect of cleaning up the request and deallocating the object. Sorry, it's a bit complicated, but I had to work within the constraints of what I was given.For example, look at
ompi_comm_request_free()
... I don't see that routine setting the status error field. Therefore, I think being overzealous here is justified. IMHO, this line is a good one and should stay. Otherwise, all internal request types used in Open MPI (and future ones that may be implemented) should be audited and eventually fixed to make sure a failing free sets the error field.