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

Stream interface suggestions #1853

Closed
wemeetagain opened this issue Jun 22, 2022 · 9 comments
Closed

Stream interface suggestions #1853

wemeetagain opened this issue Jun 22, 2022 · 9 comments

Comments

@wemeetagain
Copy link
Member

  1. Remove abort and merge behavior with close. (Behavior would be similar to muxer.close.) If the error is provided, the behavior is like abort is now. If the error is not provided, the behavior is like close now.

  2. Rename reset to destroy. Current naming is somewhat confusing.

Thoughts from reviewing ChainSafe/js-libp2p-yamux#2 with @dapplion

@achingbrain
Copy link
Member

Definitely agree the name reset is a bit weird, it says to me that you can start using the stream again after it's been called, or maybe it empties the message buffers or something which is not the case.

close/closeRead and closeWrite signal a graceful end to the stream (so no passed error). abort signals an error at the local end, reset an error at the remote end.

Locally reset closes the stream and emits a generic error to the stream handler that's pulling data from the stream - it should only really be called by the muxer in response to an incoming reset message and not by stream handler code. I do wonder if it should be removed from the interface - muxer implementations would have their own view on the streams they create so they can invoke methods not in the public interface.

abort sends the reset message to the remote, closes the stream and emits the passed error to the stream handler that's pulling data from the stream - this can be called by stream handler code.

So there are a lot of ways to close the stream, but they all do different things.

You could merge abort with close, but internally you're immediately branching on if an error was passed to either send a "close" or a "reset" message.

It might make sense to merge the two if we remove .reset from the public interface as well, then there's only close(err?), closeRead() and closeWrite() which is a bit clearer.

@dapplion
Copy link
Contributor

Agree!

@zargarzadehm
Copy link

I want to use libp2p for a production service but I couldn't find any doc about best practices for creating and closing connections and streams, for example: When I should close a stream? is it ok to keep open the stream for sending my messages all the time or after sending each message, should I close the stream?
How much is the maximum number of streams for each connection?

I developed a sample code but I got some errors and couldn't find the reason for that!
I have attached them below!

photo_2022-08-31 22 28 42

photo_2022-08-31 22 28 37

@wemeetagain
Copy link
Member Author

When I should close a stream? is it ok to keep open the stream for sending my messages all the time or after sending each message, should I close the stream?

It really depends on the needs of your application.
You may need to be continuously feeding a stream of data to peers. In that case it may make sense to keep a single stream per peer open for that. An example of this pattern is the various pubsub protocols, gossipsub and floodsub.
If your app has more of a request/response style interaction, you might structure things where each type of request is a separate protocol, and would open and close a new stream that is just for that individual request/response.

How much is the maximum number of streams for each connection?

Mplex has limits which can be configured, see https://github.com/libp2p/js-libp2p-mplex/blob/master/src/index.ts#L5

Libp2p also has stream limits which can be configured per-protocol, see https://github.com/libp2p/js-libp2p-interfaces/blob/master/packages/interface-registrar/src/index.ts

@zargarzadehm
Copy link

zargarzadehm commented Sep 5, 2022

When I should close a stream? is it ok to keep open the stream for sending my messages all the time or after sending each message, should I close the stream?

It really depends on the needs of your application. You may need to be continuously feeding a stream of data to peers. In that case it may make sense to keep a single stream per peer open for that. An example of this pattern is the various pubsub protocols, gossipsub and floodsub. If your app has more of a request/response style interaction, you might structure things where each type of request is a separate protocol, and would open and close a new stream that is just for that individual request/response.

How much is the maximum number of streams for each connection?

Mplex has limits which can be configured, see https://github.com/libp2p/js-libp2p-mplex/blob/master/src/index.ts#L5

Libp2p also has stream limits which can be configured per-protocol, see https://github.com/libp2p/js-libp2p-interfaces/blob/master/packages/interface-registrar/src/index.ts

Thanks for your response.

Yes, I need to send continuously feeding a stream of data to all peers (broadcast) or specific peer also doesn't matter the response of each message for our application, in our case, I don't know :

What is the maximum buffer in each stream?

How much data I can send in each stream?

Is there a limit on the size of messages or not?

Is there a queue or something to ensure all messages are delivered if the buffer is stuffed?

Which policies or error handling should I consider before sending each message or opening a new stream?

Sorry for so many questions, I tried to find docs or codes to find the answer to my questions but I can't find anything about these.

@achingbrain
Copy link
Member

What is the maximum buffer in each stream?

See maxStreamBufferSize in mplex - the default is 4MB

How much data I can send in each stream?

You can send as many messages as you want, and they can be as large as you want (see next answer) and you can send data for as long as you want.

Is there a limit on the size of messages or not?

There's a maxMsgSize set in the multiplexer, this is the max size on the wire - messages larger in size will be chunked and sent as multiple messages - the default value is 1MB. For example if you write a 10MB buffer into the stream, the recipient will receive 10x 1MB buffers.

To handle this properly your protocol handler should use something like it-length-prefixed to let the remote peer know how much data to expect.

Is there a queue or something to ensure all messages are delivered if the buffer is stuffed?

mplex has a buffer for each stream that is used when the protocol handler cannot consume messages fast enough. If this buffer fills up the stream will be reset - that is, it will be closed and the remote may throw an error (this is implementation specific - js will throw in the protocol handler).

Really you don't want to buffer too many messages as it becomes an attack vector if a remote can overwhelm your protocol handler.

Which policies or error handling should I consider before sending each message or opening a new stream?

This is largely down to your application's requirements. Can you be more specific about what you'd expect to consider?

@zargarzadehm
Copy link

This is largely down to your application's requirements. Can you be more specific about what you'd expect to consider?

For example, do I need to check how much of the buffer of mplex is filled before sending each message? and if yes, HOW?

I guess it is better to start implementing this section according to your answers and after that, if I have any questions ask you.

@achingbrain
Copy link
Member

For example, do I need to check how much of the buffer of mplex is filled before sending each message?

The buffer is at the receiving end, not the sending end so there's no way to check it's status. mplex doesn't have a concept of backpressure, for that we need a more sophisticated multiplexer like yamux but that's still a work in progress.

I guess it is better to start implementing this section according to your answers and after that, if I have any questions ask you.

Sounds good! You can also use the discussion forums to get help, this issue tracker is for reporting bugs in libp2p.

@achingbrain achingbrain reopened this Sep 5, 2022
@achingbrain achingbrain transferred this issue from libp2p/js-libp2p-interfaces Jun 26, 2023
@maschad maschad moved this from 🎉Done to 🥞Weekly Candidates/Discuss🎙 in js-libp2p Jun 26, 2023
@maschad
Copy link
Member

maschad commented Aug 29, 2023

This should be fixed now. The .close() gracefully closes streams and reset is now the equivalent of abort.

@maschad maschad closed this as completed Aug 29, 2023
@github-project-automation github-project-automation bot moved this from 🥞Weekly Candidates/Discuss🎙 to 🎉Done in js-libp2p Aug 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

5 participants