-
-
Save vasco-santos/a3edcf2bbb17e6ca62fff07be256e17b to your computer and use it in GitHub Desktop.
| const ROLE = { | |
| INITIATOR: 0, | |
| RESPONDER: 1 | |
| } | |
| class Stream { | |
| constructor(iterableDuplex, connection, isInitiator = true) { | |
| /** | |
| * Stream identifier | |
| */ | |
| this.id = (~~(Math.random() * 1e9)).toString(36) + Date.now() | |
| /** | |
| * Streaming iterable duplex object | |
| */ | |
| this._iterableDuplex = iterableDuplex | |
| /** | |
| * Stream parent connection | |
| */ | |
| this.connection = connection | |
| /** | |
| * Role in the stream, initiator or responder | |
| */ | |
| this.role = isInitiator ? ROLE.INITIATOR : ROLE.RESPONDER | |
| /** | |
| * Stream timeline | |
| */ | |
| this.timeline = { | |
| openTs: Date.now(), | |
| closeTs: undefined | |
| } | |
| /** | |
| * User provided tags | |
| */ | |
| this.tags = [] | |
| /** | |
| * Stream protocol | |
| */ | |
| this._protocol = undefined | |
| } | |
| sink () { | |
| } | |
| source () { | |
| } | |
| close () { | |
| } | |
| } |
We should only call the Stream "stream", otherwise we risk the same "Connection" naming nightmare, I'd rather see something like this._iterableDuplex = iterableDuplex
Great, I will change that!
What's your thought behind protocol being in the constructor? When we create a stream we likely won't know the protocol. We won't know it until multistream-select has run on that stream and negotiate a protocol. We could set it then.
I am not familiar with the multistream-select, but when we open a stream or register a stream handler (in the libp2p-daemon-client for instance) we define the protocol we want to use. Is there something I am not following?
What are the tags for? Tagging connections is meaningful as we can say this is a Bootstrap, DHT, etc connection. Do we get value from tagging each stream?
During the discussions for the introspection/visualization tool, we discussed that it would be beneficial for users of libp2p, such as ipfs, to open streams and add tags to them, which will allow filtering by those tags to decrease the debugging time.
Should we add a reference to the streams parent connection? It could make connection and stream cleanup a bit easier, rather than having to go back down the libp2p stack to get the connection.
👍
Do you have any suggestion for computing a connectionId and streamId?
I am not familiar with the multistream-select, but when we open a stream or register a stream handler (in the libp2p-daemon-client for instance) we define the protocol we want to use. Is there something I am not following?
Correct, but this is at a higher level. The flow of this might look like:
const stream = connection.newStream()
// `Connection.newStream`
newStream (protocol) {
// Create the new stream from the multiplexer, it shouldn't care about protocols
const stream = this.multiplexer.newStream() // stream.protocol == null
this.streams.push(stream)
if (!protocol) {
// If no protocol was provided, we could just return the stream for the user to manage
return stream
}
// Create a new instance of multistream to handle protocol selection
const multistream = new Multistream.Dialer()
// Negotiate the multistream protocol
await multistream.handle(stream)
// Handshake on the desired `protocol`, this should set the protocol on the stream
await multistream.select(protocol) // stream.protocol == protocol
// Return the stream
return stream
}So while the user doesn't care, if we are returning a Stream from the multiplexer, it won't know about the protocol yet. I haven't thought through the multistream api too much yet, so that could potentially be improved as well.
Do you have any suggestion for computing a connectionId and streamId?
We could just use the same logic webrtc-star uses for intent ids. The timestamp, plus the random string should have a pretty reliable non collision rate.
const id = (~~(Math.random() * 1e9)).toString(36) + Date.now()
Thanks for the feedback @jacobheun , I now agree with you regarding the protocol!
Updated the gist code according to the suggestions
this.connectionId = connectionId
Any reason to not just make this the actual connection reference? Just thinking about usability here. If I have a stream and want to get the connection, I'll have to take the connectionId and then look up the connection via the Switch. I don't think it's critical, just a potential usability improvement.
No reason, I will change to the connection reference!
@alanshaw do you envision sink and source as you have in js-libp2p-websockets?
In this case, we need to pass the options to this stream class for the abortable, and in newStream for the connection.
We should only call the
Stream"stream", otherwise we risk the same "Connection" naming nightmare, I'd rather see something likethis._iterableDuplex = iterableDuplexWhat's your thought behind
protocolbeing in the constructor? When we create a stream we likely won't know the protocol. We won't know it until multistream-select has run on that stream and negotiate a protocol. We could set it then.What are the tags for? Tagging connections is meaningful as we can say this is a
Bootstrap,DHT,etcconnection. Do we get value from tagging each stream?Should we add a reference to the streams parent connection? It could make connection and stream cleanup a bit easier, rather than having to go back down the libp2p stack to get the connection.