fix: Close stream after sink

[MarcoPolo]

Draft PR because I want to discuss this first before adding a proper error if you call sink twice and some tests.

I was surprised to learn that the stream muxers close the write side of the stream when they finish draining a sink. Is this behavior documented anywhere? The streaming-iterables.md gist doesn't mention this behavior. This also explains why I only noticed this bug on webtransport.

I think it is valid to want to pipe two sources into the same stream, but this behavior would prohibit you from doing that without making another generator that reads from the two streams.

I don't really want to change the existing muxers, so I will likely merge this. But I just want to understand what the reasoning is for this behavior and if makes something better. I think at the very least it's a bit surprising to users (it was to me).

cc @achingbrain for your js-libp2p expertise :)

[achingbrain]

I think the primary motivation was to make it easy to reason about when streams are done, though a lot of the semantics were inherited from pull-streams which were used extensively before async iterators.

I think it is valid to want to pipe two sources into the same stream

I would mildly disagree at an API level as it's hard to know when there might be a third source or a forth source.

If you want to pipe multiple things it's trivial to use a module like it-merge if you don't care about ordering or create in inline generator that yields each source one after another if you do. If you don't know how many sources you have you can use it-pushable instead though there's still an open question around back pressure and that module.

It does sound like there may be some missing tests in the stream muxer compliance tests at the very least if this isn't documented anywhere.

[MarcoPolo]

It does sound like there may be some missing tests in the stream muxer compliance tests at the very least if this isn't documented anywhere.

I should add stream muxer compliance tests to this package. This isn't easy since we don't have a listener and we're a hybrid stream-muxer transport.

[achingbrain]

I'm adding a node listener based on @fails-components/webtransport so this should become a lot easier in the very near future. Almost got a PR ready - it's passing about 1/3 of the transport compliance tests so far.

[MarcoPolo]

I found this pattern: https://github.com/ChainSafe/it-handshake/blob/bdea8b194bee756eb68390c157b130e71d279656/src/index.ts#L26

I wonder if we do something similar in other places. This pattern assumes that the we will only ever call sink once. If this pattern is very common, it would make it harder to change to having the owner of stream close the stream (rather than the sink).

[MarcoPolo]

Merging this when CI passes. No blocking comments left, and it would be good to have webtransport implement the behavior other transports do here.

[github-actions[bot]]

:tada: This PR is included in version 1.0.5 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket: