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

Re-write interface to buffer streams internally #154

Open
thomaseizinger opened this issue May 16, 2023 · 1 comment
Open

Re-write interface to buffer streams internally #154

thomaseizinger opened this issue May 16, 2023 · 1 comment

Comments

@thomaseizinger
Copy link
Contributor

thomaseizinger commented May 16, 2023

Once #150 is solved, we more or less have back-pressure in yamux. See libp2p/specs#471 (comment) for an explanation.

To make sure applications are compliant with this, we should buffer the inbound streams internally. This allows us to align the buffer size with the ACK backlog.

The interface I'd like to see is:

impl Connection<S> {
    /// Allow the connection to make progress by sending pending frames and reading data from the socket.
    ///
    /// This will buffer up to 256 new inbound streams and immediately reset any additional ones.
	pub fn poll(&mut self, cx: &mut Context<'_>) -> Poll<Result<()>>;

    /// Attempt to open a new outbound stream.
    ///
    /// This will suspend in case we already have 256 unacknowledged outbound streams.
	pub fn poll_new_outbound(&mut self, cx: &mut Context<'_>) -> Poll<Result<Stream>>;

    /// Return the next inbound stream.
    ///
    /// This will return streams from an internal buffer that gets populated as part of [`Connection::poll`].
    /// In case, the buffer is empty, this will return [`Poll::Pending`].
	pub fn poll_next_inbound(&mut self, cx: &mut Context<'_>) -> Poll<Result<Stream>>;
}
@thomaseizinger
Copy link
Contributor Author

I've tried this and I am not sure it is worth the refactoring, will put on hold.

The advantage of the current interface is that it is quite easy to determine when the connection is closed: poll_next_inbound returns None.

With the above interface, it is not obvious that this is what we should do. Because we have ConnectionError::Closed, it is equally feasible for all the above operations to now fail in case the connection is closed.

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

No branches or pull requests

1 participant