feat!: close streams gracefully #57
Conversation
- Updates all libp2p related deps - Refactors `YamuxStream` class to extend `AbstractStream` similar to other stream muxers - Stream close methods are now async BREAKING CHANGE: stream close methods are now asyc, requires libp2p@0.46.x or later
achingbrain
force-pushed
the
feat/close-streams-gracefully
branch
from
bdc198f
to
61ba47b
Compare
src/stream.ts
Outdated
| }, | ||
| onAbort: () => { | ||
| this.readState = HalfStreamState.Reset | ||
| this.writeState = HalfStreamState.Reset |
There was a problem hiding this comment.
it seems before we were checking the read/write states before overwriting them.
Eg: if (this.readState === Open) this.readState = Reset
Not sure if that is still necessary or not
There was a problem hiding this comment.
I don't think it's necessary since aborting a stream closes if for reading and writing.
I guess you could close the read end (for example) then abort but I don't know if this is a big deal or not.
The super class has similar .readStatus/.writeStatus fields so maybe these can be removed?
There was a problem hiding this comment.
Yeah, they don't seem to be used anywhere now
There was a problem hiding this comment.
I've removed them and updated the tests to assert on .readStatus/.writeStatus instead.
| /** | ||
| * Send a data message to the remote muxer | ||
| */ | ||
| async sendData (buf: Uint8ArrayList, options: AbortOptions = {}): Promise<void> { |
There was a problem hiding this comment.
where is this getting called?
There was a problem hiding this comment.
In the .sink method of AbstractStream
| switch (this.state) { | ||
| case StreamState.Finished: | ||
| return | ||
| case StreamState.Init: | ||
| // we haven't sent anything, so we don't need to send a reset. | ||
| break | ||
| case StreamState.SYNSent: | ||
| case StreamState.SYNReceived: | ||
| case StreamState.Established: | ||
| // at least one direction is open, we need to send a reset. | ||
| this.sendReset() | ||
| break |
There was a problem hiding this comment.
Is this behavior still respected?
There was a problem hiding this comment.
The stream is opened on the remote by sending the first window update. This happens in the .newStream method on the yamux muxer before the stream is returned to the caller, so there's no way to call .abort(err) on the stream before the state has gone from StreamState.Init to StreamState.SYNSent.
Similarly .state is updated to StreamState.Finished after the readable and writable ends are both closed at which point .abort(err) is a no-op so this method is not called.
| * Send a message to the remote muxer informing them a new stream is being | ||
| * opened | ||
| */ | ||
| async sendNewStream (): Promise<void> { |
There was a problem hiding this comment.
I guess we don't need this because we sent a SYN flag along with the first data to be sent? maybe worth commenting here
There was a problem hiding this comment.
Sort of, yeah - the stream is opened by the .newStream method in the muxer before any data is sent.
Was this intentional? If the sendWindowUpdate call is removed from the muxer it can switch to the behaviour of the other muxers which is to lazily signal opening the stream before the first data message is sent, in which case this method just needs to call this.sendWindowUpdate().
Though that can be done in a follow-up if it's desirable.
There was a problem hiding this comment.
I've updated the method comment to this effect.
| name?: string | ||
| stat: StreamStat | ||
| metadata: Record<string, any> | ||
|
|
||
| state: StreamState |
There was a problem hiding this comment.
Yes, for example in .getSendFlags
## [5.0.0](v4.0.2...v5.0.0) (2023-08-03) ### ⚠ BREAKING CHANGES * stream close methods are now asyc, requires libp2p@0.46.x or later * chore: pr comments * chore: remove readState/writeState as they are not used any more ### Features * close streams gracefully ([#57](#57)) ([2bd88a8](2bd88a8))
|
🎉 This PR is included in version 5.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
YamuxStreamclass to extendAbstractStreamsimilar to other stream muxersBREAKING CHANGE: stream close methods are now asyc, requires libp2p@0.46.x or later