matryer / vice

Go channels at horizontal scale (powered by message queues)
https://medium.com/@matryer/introducing-vice-go-channels-across-many-machines-bcac1147d7e2
Apache License 2.0
1.55k stars 79 forks source link

Have you considered channels as call arguments rather than return arguments #5

Closed GeorgeMac closed 5 years ago

GeorgeMac commented 7 years ago

Firstly, this is a lovely library. Love the test harness for implementers. I can see this helping people get started with queueing technologies really quickly.

Just a quick thought. Have you considered modeling the channel based functions in your interface as call arguments, rather that return arguments? I see this a lot in good channel practice.

For example:

// Transport provides message sending and receiving
// capabilities over a messaging queue technology.
// Clients should always check for errors coming through ErrChan.
type Transport interface {
    // Receive gets a channel on which to receive messages
    // with the specified name.
    Receive(name string, chan<- []byte) 
    // Send gets a channel on which messages with the
    // specified name may be sent.
    Send(name string, <-chan []byte)
    // ErrChan gets a channel through which errors
    // are sent.
    ErrChan(chan<- error)

    // Stop stops the transport. StopChan will be closed
    // when the transport has stopped.
    Stop()
    // Done gets a channel which is closed when the
    // transport has successfully stopped.
    Done() chan struct{}
}

Note that we have to flip the directions around on the channels. I think it could address some limitations of control for the consumer. For example, security that the implementation won't close the thing it is sending on or control over the channels buffer size.

For receive operations now you can expect the Receive function to potentially send or close, but not consume from your channel. That is purely the callers responsibility and they can set the buffer size.

For the Send operation, the consumer of this library is in control of when to close. The current implementation gives no guarantees to the user that the implementation of vice.Transport won't close the channel it is sending on, which would cause a panic. Enforcing that the implementation takes a receive only channel, means the transport can't close it, only stop consuming it.

Another recommendation I would have would be let Receive and Send take a context.Context and remove the Done() chan struct{} in favor of the Done on context.Context.

Otherwise, love the idea and might take a crack at a queue implementation 👍

matryer commented 7 years ago

@GeorgeMac Thanks very much.

(For discussion on Context, see https://github.com/matryer/vice/issues/10)

I think the interface you suggest would be better for more advanced users for the reasons you mentioned, but not sure about programmers new to channels etc.

But let's sketch some samples of code to see how users would use that new interface:

transport := NewTransportForSomething(ctx)
greetings := make(chan []byte)
names := make(chan []byte)
transport.Receive("names", names)
transport.Send("greetings", greetings)
for name := range names {
  greetings <- []byte("Hello "+string(name))
}

It's not so bad. This is a trade-off between simplicity of use, and power.

Interested what @dahernan @jasonhancock @piotrrojek think too.

GeorgeMac commented 7 years ago

I had the same thought process. It comes down to your target consumer. Those more familiar with channel semantics and those that want to just get going with a queue implementation.

I think upon reflection the most important call to shuffle about is the Send(name string, <-chan string) function, because you want to force implements of the interface to not close or send themselves. This way the caller is guaranteed by the compiler that they are safe to interact with the channel without the risk of a panic, because they try to send/close on an already closed channel.

I guess it may be possible to try and assert this doesn't happen in the test harness. However, that could be very difficult to assert deterministically.

For Receive it is less important and in fact by only returning a recv only channel, the consumer of this library cannot close the channel and cause the implementation of the interface to panic by unwittingly sending on a closed channel. So leaving Receive as is might be a more sane approach.