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

connections: Document stream behaviour #469

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

thomaseizinger
Copy link
Contributor

@thomaseizinger thomaseizinger commented Oct 14, 2022

An initial draft for documenting the behaviour of libp2p streams. Feedback welcome!

Open questions

  • The mplex spec differs in regards to how reading from closed streams is handled. Which one is the better behaviour? What mplex specifies or what I wrote here? I took inspiration from our current implementations in rust-libp2p.
  • Do we need specify more behaviour?
  • The "Definitions" section specifies "They can also be "half closed", meaning that a stream can be closed for writing data but still open to receiving data and vice versa." I am not sure about the "vice-versa" part, i.e. actively closing the read-half but still writing to the stream. Neither mplex nor yamux support this. The Rust implementation doesn't have an interface for this. I am not sure about others. We built it for WebRTC with inspiration from QUIC but personally don't know when I'd use it.

Fixes #466.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Thanks for documenting this! Very much appreciated.

Minus the comments, I think this is good to go. Bonus would be to document the behavior of QUIC, WebRTC, mplex and yamux here as well.

@marten-seemann want to give this a review as well?


Writing to a write-closed stream SHOULD fail with an error clearly signalling the closed state.

Reading from a read-closed stream SHOULD succeed by reading 0 bytes.
Copy link
Member

Choose a reason for hiding this comment

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

Intuitively this is to implementation specific. E.g. some may express this through a special event, some might express this through 0 bytes. I don't think we should dictate the way of how an implementation expresses this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
Reading from a read-closed stream SHOULD succeed by reading 0 bytes.
Reading from a read-closed stream SHOULD succeed.

Would that be better?

Comment on lines +293 to +294
This allows for a robust "read to end"-behaviour.
Layers on top of the stream MAY fail with an unexpected EOF error in case more data was expected.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this needs to be in the specification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you just omit it? I think I'd find it helpful because it explains, why we want read operations on closed streams to be successful. Without this, people may rightfully ask, why reading from a read-closed stream shouldn't fail?

I am happy to reword it, but I think it is useful to have in the spec in one form or another.


Upon receiving a stream reset, any buffered data in the stream MAY be discarded.

Reading and writing to a reset stream SHOULD fail with an error clearly signalling the reset.
Copy link
Member

Choose a reason for hiding this comment

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

Cross referencing a past discussion for the curious reader here, namely that QUIC only closes the write side.

#412 (comment)

I don't have a strong opinion here. Maybe @marten-seemann does. Ideally we would be consistent across the different multiplexers. At least we should document the difference here.

Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

As I've explained on the WebRTC PR, I don't think we should define our stream state machine to match the behavior of legacy stream muxers like yamux or mplex. I'd like our stream state machine to take advantage of what QUIC has to offer.
By virtue of that, make it possible to map protocols like HTTP onto libp2p. The current proposal would prohibit this.

@thomaseizinger
Copy link
Contributor Author

As I've explained on the WebRTC PR, I don't think we should define our stream state machine to match the behavior of legacy stream muxers like yamux or mplex. I'd like our stream state machine to take advantage of what QUIC has to offer. By virtue of that, make it possible to map protocols like HTTP onto libp2p. The current proposal would prohibit this.

That does make sense.

Would you still keep it symmetric though? More specifically, can you reset a read-only stream? From this conversation it appears that QUIC cannot do that. I am curious to understand why. Does "closing" a read-only stream always communicate the same intent?

@mxinden
Copy link
Member

mxinden commented Nov 29, 2022

@thomaseizinger @marten-seemann what are the next steps here? What do you think of documenting the QUIC stream behavior as the reference plus how the other stream muxers deviate from that? While not ideal, it would document the status quo and where we are heading.

@marten-seemann
Copy link
Contributor

What do you think of documenting the QUIC stream behavior as the reference plus how the other stream muxers deviate from that? While not ideal, it would document the status quo and where we are heading.

Sounds good to me.

@thomaseizinger
Copy link
Contributor Author

@thomaseizinger @marten-seemann what are the next steps here? What do you think of documenting the QUIC stream behavior as the reference plus how the other stream muxers deviate from that? While not ideal, it would document the status quo and where we are heading.

I would still like to understand why resetting a read-stream is something that is unsupported. It seems like an obvious step from the two-stream mental model that you can reset both to signal an abrupt termination.

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

Successfully merging this pull request may close these issues.

Carify usage and behaviour of stream resets
3 participants