Skip to content
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

Wrap tokio channels & Information Package Def #75

Merged
merged 3 commits into from
Nov 4, 2024

Conversation

191220029
Copy link
Collaborator

Signed-off-by: XiaolongFu-NJU <522023330025@smail.nju.edu.cn>
Comment on lines 12 to 19
/// ## Implementaions
/// - `blocking_recv_from`: call `blocking_recv` of the receiver by the given `NodeId`. Returns `Ok(Content)`
/// if message received; returns `Err(RecvErr)` if the given `NodeId` is invalid, no message is available to recv,
/// or err occurs.
/// - `recv_from`: receives the next value for this receiver by the given `NodeId` asynchronously. Returns `Ok(Content)`
/// if message received; returns `Err(RecvErr)` if the given `NodeId` is invalid, or no message is available to recv,
/// or err occurs.
/// - `close`: close the channel by the given `NodeId`, and remove the channel in this map.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This duplicates the actual method documentation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will remove the duplicated documentation in the next commit.

/// pakages are dropped on this receiver's side.
#[derive(Debug)]
pub enum RecvErr {
ChannelNExist,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like ChannelNExist isn't a very clear variant name. How about something like InvalidNodeId, NoSuchChannel, or NoMatchingChannel?

Comment on lines 24 to 26
/// Call `blocking_recv` of the receiver of the given `NodeId`. Returns `Ok(Content)`
/// if message received; returns `Err(RecvErr)` if the given `NodeId` is invalid, no message is available to recv,
/// or err occurs.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Having the documentation of the return value in the same paragraph as the method's purpose feels somewhat hard to read to me. I'm also not sure if there's much value in documenting the success case, since it's ultimately just "if no errors occurred." What do you think about something like the following?

Suggested change
/// Call `blocking_recv` of the receiver of the given `NodeId`. Returns `Ok(Content)`
/// if message received; returns `Err(RecvErr)` if the given `NodeId` is invalid, no message is available to recv,
/// or err occurs.
/// Perform a blocking receive on the incoming channel from `NodeId`.
///
/// # Errors
///
/// This method can return the following errors:
///
/// - `RecvErr::ChannelNExist` if the given `NodeId` was invalid.
/// - `RecvErr::Closed` if all senders were dropped.
/// - `RecvErr::Lagged(x)` if some messages were dropped before the receiver could receive
/// them. Contains the number of messages lost.

Though it also doesn't feel great to essentially duplicate the documentation for the RecvErr type on every method that returns it since that's a recipe for partially updated docs, but it would be nice to make it clearer that InChannel::recv will never return RecvErr::ChannelNExist.

I wonder if it would be better to have one RecvErr for InChannels and a separate one for InChannel. Right now if someone were to call InChannels::get and then use InChannel directly they would need to handle the RecvErr::ChannelNExist case even though it should never be able to happen.

Copy link
Collaborator Author

@191220029 191220029 Oct 31, 2024

Choose a reason for hiding this comment

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

I prefer your suggestion to modify the document. I will update the documentation here in my next commit.

For the err type, we want users to receive messages from other nodes directly through InChannels, without caring what InChannel is. That's why I defined the methods implemented for InChannel as private items. Maybe we can define InChannel as a private item too?

So I think we don't have to create a separate Err type for InChannel? because on the user side they should not know the existence of InChannel.

Signed-off-by: Xiaolong Fu <75103649+191220029@users.noreply.github.com>
pub enum SendErr {
NoSuchChannel,
MpscError(mpsc::error::SendError<Content>),
BcstError(broadcast::error::SendError<Content>),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I checked the doc of tokio mpsc & broadcast and found that we can merge the error types SendErr::MpscError and SendErr::BcstError into a single ChannelClosed, because the send operation of mpsc or broadcast will only fail if there is no active receiver (i.e. closed channel).
More detailed info can be found in the src code of tokio: mpsc::SendError and broadcast::SendError

Suggested change
BcstError(broadcast::error::SendError<Content>),
ClosedChannel(Content),

Signed-off-by: Xiaolong Fu <75103649+191220029@users.noreply.github.com>
@fengyang-nju
Copy link
Collaborator

It looks good to me.

@genedna genedna merged commit 4e5fc0b into dagrs-dev:main Nov 4, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants