Skip to content
This repository has been archived by the owner on Nov 5, 2018. It is now read-only.

Dropping receivers disconnects the channel #106

Merged
54 commits merged into from Nov 4, 2018
Merged

Dropping receivers disconnects the channel #106

54 commits merged into from Nov 4, 2018

Conversation

ghost
Copy link

@ghost ghost commented Oct 29, 2018

NOTE: This PR is a work in progress.

The main change is: Dropping all receivers closes the channel.
See #61 for the rationale.

Other notable changes in this PR:

  • The syntax for send in select! is now send(sender, msg) -> res => body.
  • The syntax for recv in select! is now recv(receiver) -> res => body.
  • Timeout can be specified in the default case in select!.
  • The interface for the Select struct is revamped.
  • The Sender/Receiver interface is now similar to what we had in version 0.1.
  • Add special never channel that never delivers messages.

Closes #61
Closes #99
Closes #55

@ghost ghost changed the title Dropping senders disconnects the channel Dropping receivers disconnects the channel Oct 29, 2018
@ghost ghost mentioned this pull request Oct 30, 2018
@arthurprs
Copy link

This is awesome!!! I'll review once you remove the wip notice.

@ghost
Copy link
Author

ghost commented Oct 31, 2018

@arthurprs The PR is in a usable state so you can already try it out. The only thing that still needs work is documentation (don't read it, it makes no sense at the moment :)).

@cramertj @aturon Can I ask you for an opinion regarding select!? So the new syntax I went with is:

select! {
    recv(receiver) -> res => { 
        // `res` is of type `Result<T, RecvError>`.
        // Note that the `recv` operation might return an error.
        println!("recv completed with {:?}", res);
    }
    send(sender, message) -> res => {
        // `res` is of type `Result<(), SendError<T>>`.
        // Note that the `send` operation might return an error.
        //
        // In particular, this case can fire even if the channel is closed and
        // the message isn't really sent! In that case, the message is given
        // back as the `Err` case.
        println!("send completed with {:?}", res);
    }
    default => {
        // This case is optional.
        // You can also write `default() =>`.
        //
        // It's even possible to specify a timeout of type `Duration`
        // with `default(timeout) =>`.
        println!("all operations would block");
    }
}

The idea behind recv(r) -> res => and send(s, msg) -> res => is that they correspond to invocations of let res = Receiver::recv(&r) and let res = Sender::send(&s, msg). The select! then waits until any of these method invocations can proceed without blocking, or bails out through default.

An alternative syntax might be res = recv(r) => and res = send(s, msg) =>, but to me it seems a little bit uglier. @vorner said that syntax feels unnatural and I agree, although it's difficult to put my finger on what exactly bugs me with it. Perhaps because it doesn't align as nicely with default(timeout) =>.

On the other hand, val = fut => in the futures version of select! feels OK to me.

Do you agree with these syntactic choices?

@glaebhoerl
Copy link

it's difficult to put my finger on what exactly bugs me with it

Two things, in my case:

  • It's not "in chronological order". Logically we first do the operation, then we get the result, then we do something with the result. But here the first two of those are flipped.

  • There's some visual ambiguity w.r.t. associativity; even if it may be obvious after thinking about it, it's better if it's obvious without having to think about it. I'm not actually sure why this isn't also an issue for the two-arrows syntax, though at least for me it doesn't seem to be. (It's possible that in my case this is due to hard-earned intuitions from Haskell w.r.t. currying carrying over, in which case it may not generalize to other people.)

Does the macro allow actual pattern matching in the res position? That is, can you write:

select! {
    recv(my_receiver) -> Ok(value)  => { ... }
    recv(my_receiver) -> Err(error) => { ... }
    ...
}

as an alternative to doing a match/if let/? on the res inside the (in that case single) RHS block?

@ghost
Copy link
Author

ghost commented Nov 1, 2018

@glaebhoerl

Does the macro allow actual pattern matching in the res position?

The thing that goes after -> is a pattern, but if you write two recv(my_receiver) cases they will be two completely independent cases.

In your example, the compiler would complain about non-exhaustive pattern matching (for the same reason it would complain about let Ok(value) = r.recv();).

The way to pattern-match on the result in select! is:

select! {
    recv(my_receiver) -> res => match res {
        Ok(value)  => { ... }
        Err(error) => { ... }
    }
    ...
}

Writing res and arrows twice is a bit annoying, but it is what it is. :/

@ghost
Copy link
Author

ghost commented Nov 2, 2018

@arthurprs Ok, the documentation has been updated. I'll let this PR sit for a few days and then merge it and publish a new version.

Copy link

@arthurprs arthurprs left a comment

Choose a reason for hiding this comment

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

I'm very happy about this, I couldn't find anything weird but this is my first look at the internals.

Something that came to mind was to (eventually) change some internal asserts and unwraps into debug_assert and debug_unwrap.

@ghost
Copy link
Author

ghost commented Nov 2, 2018

@arthurprs Thanks for taking a look at the code!

I'm very happy about it, too. This PR also simplifies the selection mechanism so now the select! macro nicely desugars into Select. I have a feeling we might've finally found the sweet spot in the whole design space.

Something that came to mind was to (eventually) change some internal asserts and unwraps into debug_assert and debug_unwrap.

Yes, there are a few places where we could do that.

If it doesn't affect performance, I often choose assert! over debug_assert! in and around unsafe code. If there's a bug in unsafe code, assert! might prevent the program from running into UB even in release mode. Makes me sleep better at night. :)

@ghost ghost merged commit 16d4601 into crossbeam-rs:master Nov 4, 2018
@ghost ghost deleted the refactor branch November 4, 2018 12:53
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
3 participants