js-libp2p icon indicating copy to clipboard operation
js-libp2p copied to clipboard

feat!: muxed streams as web streams

Open achingbrain opened this issue 2 years ago • 6 comments

Refactors streams from Duplex async iterables:

{
  source: Duplex<AsyncGenerator<Uint8Array, void, unknown>, Source<Uint8Array | Uint8ArrayList>, Promise<void>
  sink: (Source<Uint8Array | Uint8ArrayList>) => Promise<void>
}

to ReadableWriteablePair<Uint8Array>s:

{
  readable: ReadableStream<Uint8Array>
  writable: WritableStream<Uint8Array>
}

Since the close methods for web streams are asynchronous, this lets us close streams cleanly - that is, wait for any buffered data to be sent/consumed before closing the stream.

We still need to be able abort a stream in an emergency, so streams have the following methods for graceful closing:

stream.readable.cancel(reason?: any): Promise<void>
stream.writable.close(): Promise<void>

// or

stream.close(): Promise<void>

..and for emergency closing:

stream.abort(err: Error): void

Connections and multiaddr connections have the same close/abort semantics, but are still Duplexes since making them web streams would mean we need to convert things like node streams (for tcp) to web streams which would just make things slower.

Transports such as WebTransport and WebRTC already deal in web streams when multiplexing so these no longer need to be converted to Duplex streams so it's win-win.

Fixes #1793

BREAKING CHANGE: muxed streams are now ReadableWriteablePair web streams and not Duplex async iterables

achingbrain avatar Jun 23 '23 12:06 achingbrain

Still a draft as there's more work to do, tests aren't passing yet, etc - one obvious thing is this PR adds yamux to the monorepo which accounts for most of the +/- line changes - this is just temporary to ensure compatibility and will be removed before merging and a separate PR opened with the changes there, though release will be a bit chicken & egg because of the circular dependency between yamux and here and yamux.

Gossipsub will probably also need adding to this PR temporarily for the interop tests to pass.

Opening early to get some feedback on the stream/connection interface changes.

achingbrain avatar Jun 23 '23 12:06 achingbrain

we need to convert things like node streams (for tcp) to web streams which would just make things slower.

I'm not convinced that they would be slower. We already have some conversion which uses stream-to-it to convert the node stream to a duplex. We could instead use stream.Duplex.toWeb to convert the node stream to a web stream.

We would need actual benchmarks to know if it is slower or not.

wemeetagain avatar Jun 23 '23 18:06 wemeetagain

@wemeetagain I put a simple benchmark together here - https://gist.github.com/achingbrain/7e65ca748326d3b87b81508020a3321d

Results are on there but TLDR is node streams are fastest, web streams are a bit slower and node streams wrapped in web streams are slower still.

Interestingly adding type: 'bytes' to web streams made them slower which is not what I would have expected. That may need a bit more investigation.

achingbrain avatar Jun 23 '23 22:06 achingbrain

On the basis of the benchmarks showing a performance degradation with the switch to web streams, is this something we actually want to do? My gut feeling would be no, not until web streams close the performance gap to node streams/duplex iterables.

achingbrain avatar Jun 26 '23 08:06 achingbrain

On the basis of the benchmarks showing a performance degradation with the switch to web streams, is this something we actually want to do? My gut feeling would be no, not until web streams close the performance gap to node streams/duplex iterables.

:+1:

wemeetagain avatar Jun 26 '23 14:06 wemeetagain