Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: muxed streams as web streams #1847

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

achingbrain
Copy link
Member

@achingbrain achingbrain commented Jun 23, 2023

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

Refactors streams from duplex async iterables:

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

to `ReadableWriteablePair<Uint8Array>`s:

```js
{
  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:

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

// or

stream.close(): Promise<void>
```

..and for emergency closing:

```js
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
@achingbrain
Copy link
Member Author

achingbrain commented Jun 23, 2023

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.

*/
export interface Stream extends Duplex<AsyncGenerator<Uint8ArrayList>, Source<Uint8ArrayList | Uint8Array>, Promise<void>> {
export interface RawStream extends ByteStream {
Copy link
Member Author

@achingbrain achingbrain Jun 23, 2023

Choose a reason for hiding this comment

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

Don't know it RawStream is the right name - it's a stream that's been opened by the muxer but mss has not been used to negotiate a protocol for it yet. In an ideal world this type will never make it to the user.

@wemeetagain
Copy link
Member

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.

@achingbrain
Copy link
Member Author

achingbrain commented Jun 23, 2023

@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
Copy link
Member Author

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.

@wemeetagain
Copy link
Member

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.

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🧊Icebox🥶
Development

Successfully merging this pull request may close these issues.

Cleanly closing streams
2 participants