async_channel: simplify logic to obtain Sender/Receiver endpoints

[amab8901]

Addresses 2nd checkbox on #715. Is that checkbox complete now?

Continuation of #762

[kevinaboos]

Is that checkbox complete now?

No, not quite. Did you perhaps forget to commit some other changes from your local machine? The only change I currently see in this PR is a couple lines of doc comments on the Sender::receiver() and Receiver::sender() methods.

The change we actually want to make is to ensure that the channel's sender and receiver counts are properly updated when obtaining a new instance of a sender or receiver. The best way to do this would be to re-use the code you recently added to the Clone impl in #707.

Let us know if you need more hints or would prefer that we take over the implementation task directly.

[amab8901]

Is it good now?

Let us know if you need more hints or would prefer that we take over the implementation task directly.

One of the reasons I'm contributing to this project is because I wanna gain experience and become a better Rust developer so I may one day hopefully get a Rust job. So hints would be preferable over the 2nd option.

[kevinaboos]

Is it good now?

Functionally it appears correct! Nice work.

Now, the next steps to improve things would be:

1. Deduplicate the code that is shared between the Clone impl and your new methods.
2. Fully document your new methods
3. Replace the existing Sender::receiver() and Receiver::sender() with your new methods, and ensure that all other code that depends on those old methods is changed to use your new methods. In other words, running make full should succeed.

Let us know if you need more hints or would prefer that we take over the implementation task directly.

One of the reasons I'm contributing to this project is because I wanna gain experience and become a better Rust developer so I may one day hopefully get a Rust job. So hints would be preferable over the 2nd option.

Of course, I surmised as much, that's why I'm happy to oblige you in the learning process. I appreciate your perseverance too!

[amab8901]

good now?

[kevinaboos]

Did you have an issue with git again? If you take a look at the Files changed tab, you'll see that the only changes that are currently there are some formatting/whitespace changes. Something didn't go right.

[kevinaboos]

The logic from this commit was correct, we were just asking you to deduplicate the code logic now shared between Sender::obtain_receiver() and Receiver::clone(), as well as Receiver::obtain_sender() and Sender::clone().

One way you could do that is by extracting that logic into a separate function that is then called in both Sender::obtain_receiver() and Receiver::clone() (and vice versa).

[amab8901]

s. Som

oh, that's what you meant by "deduplicate". I thought "deduplicate" meant "modify them so that they are different 😆 Ok I'll try a new iteration

[amab8901]

Ok, so I've been trying to deduplicate the obtain_sender(), obtain_receiver() and the Clone implementations. Here are my findings:

They don't seem to be duplicate:

• Receiver produces Receiver via Clone implementation, and produces Sender via obtain_sender() method.
• Sender produces Sender via Clone implementation, and produces Receiver via obtain_receiver() method.
  Idk how Sender and Receiver could call each other's methods. They seem isolated from each other.

Are you sure that they are duplicate and need to be deduplicated? I'm not sure how to deduplicate it 🤔

[kevinaboos]

They don't seem to be duplicate:

The code in Receiver::clone() and Sender::obtain_receiver() is identical. Similarly, the code in Sender::clone() and Receiver::obtain_sender() is also identical. The goal is to refactor those into a single inner function each, which is then called by those respective outer functions.

• Sender produces Sender via Clone implementation, and produces Receiver via obtain_receiver() method.
  Idk how Sender and Receiver could call each other's methods. They seem isolated from each other.

Now, I could directly provide the implementation for this, but you have previously expressed the desire to learn, so I'll just provide one hint. Where should the new inner function go? Hint: what do the Sender and Receiver have in common? They are indeed separate types, but they are not fully "isolated" from each other.

Also, please take a moment to study a git usage guide. There's no need for continuous force pushes, it makes it very hard to track the history of your changes since they keep getting removed and overwritten.

Finally, before asking for another review pass, kindly take a moment to review your own code in the Files changed tab, and also ensure that all checks are passing.

[amab8901]

is macro expansion a feasible approach for deduplicating? I tried and it didn't work. I'm wondering whether I did it the wrong way or whether it's actually not possible.

Here is my attempt:

macro_rules! add_sender {
    ($self:expr) => {
        if $self.channel.sender_count.fetch_add(1, Ordering::SeqCst) == 0 {
            $self.channel.channel_status.store(ChannelStatus::Connected);
        }
        Sender { channel: $self.channel.clone() }
    }
}
[...]
    /// Obtain sender from the given channel 
    pub fn obtain_sender(&self) -> Sender<T> {
        add_sender!(self);
    }

I get this error:

error[E0308]: mismatched types
   --> kernel/async_channel/src/lib.rs:507:36
    |
507 |     pub fn obtain_sender(&self) -> Sender<T> {
    |            -------------           ^^^^^^^^^ expected struct `Sender`, found `()`
    |            |
    |            implicitly returns `()` as its body has no tail or `return` expression
    |
    = note: expected struct `Sender<T>`
            found unit type `()`

[amab8901]

here is the function I'm trying to create to combine clone sender and obtain receiver for Sender struct:

    pub fn obtain_item<U: Send>(&self, item: U) -> dyn Send {
        match TypeId::of::<U>() {
            TypeId::of::<Sender<T>>() => {
                if self.channel.sender_count.fetch_add( 1, Ordering::SeqCst ) == 0 {
                    self.channel.channel_status.store( ChannelStatus::Connected );
                }
                Sender { channel: self.channel.clone() }
            },
            TypeId::of::<Receiver<T>>() => {
                if self.channel.receiver_count.fetch_add( 1, Ordering::SeqCst ) == 0 {
                    self.channel.channel_status.store( ChannelStatus::Connected );
                }
                Receiver { channel: self.channel.clone() }
            },
            _ => {}
        }
    }

Here is another non-working attempt:

    pub fn obtain_item<U: Send>(&self, item: U) -> dyn Send {
        let sender_id = TypeId::of::<Sender<T>>();
        let receiver_id = TypeId::of::<Receiver<T>>();
        match TypeId::of::<U>() {
            sender_id => {
                if self.channel.sender_count.fetch_add( 1, Ordering::SeqCst ) == 0 {
                    self.channel.channel_status.store( ChannelStatus::Connected );
                }
                Sender { channel: self.channel.clone() }
            },
            receiver_id => {
                if self.channel.receiver_count.fetch_add( 1, Ordering::SeqCst ) == 0 {
                    self.channel.channel_status.store( ChannelStatus::Connected );
                }
                Receiver { channel: self.channel.clone() }
            },
            _ => {}
        }
    }

Any feedback?

[kevinaboos]

no need for anything nearly that complex. Hint: move the core functions into the Channel type, which is the common element between Sender and Receiver.

[amab8901]

I think I finally made it

[kevinaboos]

Looks great now! Thanks, and glad we got there in the end. 👍