Skip to content

Conversation

@chnlkw
Copy link
Contributor

@chnlkw chnlkw commented Apr 27, 2025

  • Fix send and reply data with AmProto::Eager. According to the document of UCP_AM_RECV_ATTR_FLAG_DATA in
    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_nbx when receiving UCP_AM_RECV_ATTR_FLAG_DATA anymore.
  • fix doc in am.rs
  • BREAKING CHANGE: am::AmMsg.get_data() Returns None if needs to receive data, but returns Some(&[]) when data is empty.

chnlkw added 2 commits April 27, 2025 21:10
According to the document of UCP_AM_RECV_ATTR_FLAG_DATA, it should call
ucp_am_data_release after using AmData::Data.
@chnlkw chnlkw changed the title Manually drop the case AmData::Data Fix crashing by sending with AmProto::Eager Apr 27, 2025
@chnlkw chnlkw changed the title Fix crashing by sending with AmProto::Eager Fix the crashing of sending with AmProto::Eager proto Apr 27, 2025
@wangrunji0408 wangrunji0408 requested a review from Copilot April 28, 2025 04:00
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

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)];

@wangrunji0408 wangrunji0408 requested a review from yiyuanliu April 28, 2025 09:56
Copy link
Contributor

@yiyuanliu yiyuanliu left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

@wangrunji0408 wangrunji0408 merged commit bfe5896 into madsys-dev:main Apr 29, 2025
2 checks passed
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.

3 participants