Fix the crashing of sending with AmProto::Eager proto#8
Merged
wangrunji0408 merged 2 commits intomadsys-dev:mainfrom Apr 29, 2025
Merged
Fix the crashing of sending with AmProto::Eager proto#8wangrunji0408 merged 2 commits intomadsys-dev:mainfrom
wangrunji0408 merged 2 commits intomadsys-dev:mainfrom
Conversation
According to the document of UCP_AM_RECV_ATTR_FLAG_DATA, it should call ucp_am_data_release after using AmData::Data.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue with sending and replying with AmProto::Eager by using AmData::Data directly and calling ucp_am_data_release after use, while also updating the documentation and test behavior.
- Revised behavior of get_data to differentiate between messages with concrete data and those that still need to receive data
- Adjusted recv_data_vectored and the drop logic to correctly handle AmData variants
- Updated tests to iterate over different AmProto values, including None, Eager, and Rndv
Comments suppressed due to low confidence (2)
src/ucp/endpoint/am.rs:139
- The implementation of get_data returns 'Some(&[])' when self.msg.data is None, while the documentation states that it should return None if data needs to be received. Please verify and adjust the behavior to ensure consistency with the intended API design.
match self.msg.data { Some(ref amdata) => amdata.data(), None => Some(&[]), }
src/ucp/endpoint/am.rs:620
- [nitpick] Consider adding explicit test cases to verify the behavior of get_data for both scenarios — when concrete data is available and when data is expected to be received (empty case) — to improve coverage of the breaking change.
let protos = vec![None, Some(AmProto::Eager), Some(AmProto::Rndv)];
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ucx, it should use the AmData::Data directly as well as calling ucp_am_data_release after using. Also it is no need to call
ucp_am_recv_data_nbxwhen receivingUCP_AM_RECV_ATTR_FLAG_DATAanymore.am::AmMsg.get_data()ReturnsNoneif needs to receive data, but returnsSome(&[])when data is empty.