New channel interface

[dmcgowan]

Add a new single channel interface for sending receiving with support for dynamic types. Any type can contain channels or bytestreams as field values and communicate them over a channel. This is a work in progress and up for early review.

[dmcgowan]

After playing around with the interfaces some, I think the higher level Channel interface and referring to ByteStreams as io.ReadWriteCloser may not be sufficient for the high level interface. The problem I quickly ran across in receive messages, if interfaces are used in the receive struct, the message will not be able to be decoded unless an empty object is set to the interface (could just be a limitation of the decoder). This defeats the whole purpose of using interfaces in the first place. A possible alternate solution is to make libchan.Channel and libchan.ByteStream structs that are used in messages then encapsulate any typing information inside that struct. They can still have the same top level functions but the object could be created from different types. This would add an extra layer of type coding though on top of the msgpack extended types, ideally msgpack decoding could just handle using the type information to assign to an interface.

[dmcgowan]

Update with some of the features discussed in previous comment. However Channel remains to be an interface, while ByteStream is a struct. ByteStream could possibly be an interface instead, I don't see any current limitations. To get around the msgpack issue currently a modified version of the go msgpack library is being used. Because of this testing this PR may fail unless using dmcgowan/go@5d26f5f for github.com/ugorji/go/codec. Since bytestreams have now been separated from the transport, it is possible to create bytestreams of different types and send them with the transport.

[smarterclayton]

Agree ByteStream feels more like an interface at first glance - still absorbing though.

[dmcgowan]

I think making ByteStream interface is the way to go, and keep its default implementation for a transport up to the transport (spdy uses spdystream). Likewise I was thinking that the provider and listener interfaces are not needed if net.Conn is used as the extensible type, in which case the local/remote address combos can be used to reference the connections. The connections just need to be registered with the transport. This sort of interface should cover a wide variety of use cases (although unix sockets may need to be handled specially).

[dmcgowan]

Removed the top level ByteStream interface, what seems to make the most sense to use is CreateByteStream uses the transport default and returns a ReadWriteCloser. To handle more exotic types of streams, added direct support for net.Conn instead of attempting to wrap net.Conn in a lesser interface. @erikh I would appreciate your feedback on this change.

Assuming no objections on the bytestream/net.Conn interface, I would still like to get some finalization on whether Channel needs to be broken back out into Sender/Receiver. After working with @aanand on integrating the new interface into libswarm, I don't see breaking them out as actually providing anything except possibly exchanging one confusing aspect of Channels for a different one. I would really like to get an idea of which is more straightforward though.

The interface that I am requesting reviewed for those who don't want to read through all that code is this section of libchan.go

type Direction uint8

const (
    Out = Direction(0x01)
    In  = Direction(0x02)
)

type TransportSession interface {
    NewSendChannel() Channel
    WaitReceiveChannel() Channel

    RegisterConn(net.Conn)
    RegisterListener(net.Listener)
    Unregister(net.Conn)
}

type Channel interface {
    CreateByteStream() (io.ReadWriteCloser, error)
    CreateSubChannel(Direction) (Channel, error)

    Communicate(message interface{}) error
    Close() error
    Direction() Direction
}

[erikh]

Is there a reason TransportSession couldn’t be broken out into ListenerSession and ConnSession, who implement net.Listener and net.Conn? You could add a third interface for the channel stuff that these implement?

-Erik

On Jul 11, 2014, at 11:59 AM, Derek McGowan notifications@github.com wrote:

Removed the top level ByteStream interface, what seems to make the most sense to use is CreateByteStream uses the transport default and returns a ReadWriteCloser. To handle more exotic types of streams, added direct support for net.Conn instead of attempting to wrap net.Conn in a lesser interface. @erikh I would appreciate your feedback on this change.

Assuming no objections on the bytestream/net.Conn interface, I would still like to get some finalization on whether Channel needs to be broken back out into Sender/Receiver. After working with @aanand on integrating the new interface into libswarm, I don't see breaking them out as actually providing anything except possibly exchanging one confusing aspect of Channels for a different one. I would really like to get an idea of which is more straightforward though.

The interface that I am requesting reviewed for those who don't want to read through all that code is this section of libchan.go

type Direction uint8

const (
Out = Direction(0x01)
In = Direction(0x02)
)

type TransportSession interface {
NewSendChannel() Channel
WaitReceiveChannel() Channel

RegisterConn(net.Conn)
RegisterListener(net.Listener)
Unregister(net.Conn)

}

type Channel interface {
CreateByteStream() (io.ReadWriteCloser, error)
CreateSubChannel(Direction) (Channel, error)

Communicate(message interface{}) error
Close() error
Direction() Direction

}
—
Reply to this email directly or view it on GitHub.

[dmcgowan]

There is no overlap between what TransportSession does and what would be need to implement the net interfaces. I think I could be missing what you see as the intended usage of it, maybe a short gist of how you think it would be used. Right now the "Session Listener" would just take in a net.Listener and emit new TransportSessions for each accepted net.Conn. The default TransportSession implementation be a transport based off a spdy connection.

[dmcgowan]

Removed WIP tag, this PR is ready to be considered for merge

[mcollina]

I'm going through this new implementation, and I have a question regarding nested channels (which is not covered in the PROTOCOL.md file) vs bytestreams. It seems that nested channels are retrieved by looking if there is a SPDY stream with the same id as encoded in the message, while bytestreams use 'libchan-ref'. Is it final, or nested channels will be moved to 'libchan-ref' too?

From a jschan point of view, using 'libchan-ref' for everything is much easier to code, as it does not rely on low-level access to SPDY (which we do not have in node-spdy).

[dmcgowan]

The intention for channels is to not use the 'libchan-ref', which is mentioned in the PROTOCOL file as determining whether a stream is a channel or a bytestream. What is not mentioned is what would be used in its place. Since channels have a nested hierarchy, to me it makes sense to use the spdy stream ids which already have that hierarchy. In practice the hierarchy has not proven that useful yet and not currently supported in code (nothing is enforcing that a channel created by one channel cannot be sent on another, although must be the same session). However down the road I think it should be enforced and the hierarchy can be useful for debugging. As for possibly using the reference ids, the reference id does not have this hierarchy and we want to be able to use in the future to span multiple spdy sessions. Channels are more tied to their transport which makes using the stream id just less book keeping.

Does the node-spdy library you are using provide a higher level interface which abstracts the underlying streams away? I will take a look at your node implementation today and try and see what you guys are up against.

@shykes any commentary on nested channels since that is yet to be filled out in the protocol. We need to chat later this week about filling in some holes in the protocol doc.

[dmcgowan]

Moving previous discussion to #48

[dmcgowan]

@shykes moved the copy logic to the sender to allow for transparently passing io.ReadWriteClosers without needing to wrap. This also allows sending channels from different transports and transparently copying the values.

[dmcgowan]

@shykes the interface has been reduced to remove nested channels in favor of libchan.Pipe as we discussed. My first attempt to reuse the original in-mem pipe was not successful because of the need to copy between map[string]interface{} and a real struct. It is possible to do but could not be done naively as we hoped. I will add the copy logic and use the original in-mem pipe as I find time but at this point it is an improvement rather than an interface enhancement or change. It is completely functional as is.

[shykes]

LGTM