fix: Close stream after sink #23
Conversation
|
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 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 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. |
|
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. |
|
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
force-pushed
the
marco/close-after-sink-ends
branch
from
b58ee11 to
0370aa5
Compare
MarcoPolo
force-pushed
the
marco/close-after-sink-ends
branch
from
0370aa5 to
ce4cbe1
Compare
| @@ -73,7 +90,10 @@ async function webtransportBiDiStreamToStream (bidiStream: any, streamId: string | |||
| }); | |||
|
|
|||
| (async function () { | |||
There was a problem hiding this comment.
Could lose the IIFE and just wrap the contents in a try/catch?
| @@ -63,7 +74,13 @@ async function webtransportBiDiStreamToStream (bidiStream: any, streamId: string | |||
| let writerClosed = false | |||
| let readerClosed = false; | |||
| (async function () { | |||
There was a problem hiding this comment.
Could lose the IIFE and just wrap the contents in a try/catch?
There was a problem hiding this comment.
I don't think so? await writer.closed will block until the writer is closed. We could get rid of the iife by doing something like:
writer.closed.catch(err => err).then(maybeErr => { ...
Do you think that's better?
Co-authored-by: Alex Potsides <[email protected]>
Co-authored-by: Alex Potsides <[email protected]>
Co-authored-by: Alex Potsides <[email protected]>
Co-authored-by: Alex Potsides <[email protected]>
Co-authored-by: Alex Potsides <[email protected]>
|
Merging this when CI passes. No blocking comments left, and it would be good to have webtransport implement the behavior other transports do here. |
## [1.0.5](v1.0.4...v1.0.5) (2022-11-16) ### Bug Fixes * Close stream after sink ([#23](#23)) ([a95720c](a95720c))
|
🎉 This PR is included in version 1.0.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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 :)