-
Notifications
You must be signed in to change notification settings - Fork 280
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
Simple request/response #561
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. Thank you Marco for writing it up.
I have one suggestion in regards to stream closing, i.e. only requiring write-side closing not general stream closing.
I'll review tomorrow. |
This is the first time I am hearing about such a thing as an explicitly optional spec. I thought that is the default? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for writing this up. I have some ideas & questions, let me know what you think :)
Co-authored-by: Max Inden <[email protected]>
Co-authored-by: Max Inden <[email protected]>
d95cd25
to
1e1c96d
Compare
go-libp2p-kad-dht is compatible with this, despite it doing some pipelining. It tries to pipeline, but if that fails it will use a new stream. I think we should change the go-libp2p-kad-dht implementation to not pipeline. And I think this is still backwards compatible as existing clients will try a new stream if the server closes the stream or when the client fails to read the response for the pipelined request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great effort Marco!
This is now much clearer for me. You might want to rename the file still before merging.
|
||
## How to map to a libp2p stream | ||
|
||
Each request and response should happen in a single stream. There SHOULD NOT be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each request and response should happen in a single stream. There SHOULD NOT be | |
Each request and response should happen in a single stream. There MUST NOT be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For backwards compatibility, this can't be a MUST.
I'd like to have existing request/response over streams (like identify), to already be compliant with this spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previous discussion: #561 (comment)
(signalling EOF to the peer). After handling the response, the client SHOULD | ||
close the stream. After sending the response the server SHOULD close the stream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence doesn't make sense, the client already closed the stream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both sides can close the stream, right?
@mxinden / @thomaseizinger I believe this is what rust-libp2p does, could you confirm please?
The motivation for this PR is to introduce a way of defining request response style application protocols without requiring it to be built on top of HTTP semantics directly. This is meant to be minimal and very agnostic as to what the application protocol does. For example, I'm not defining a way to include metadata or headers here. This is to keep it minimal, but also to preserve backwards compatibility.
I'm open to naming suggestions for this instead of "simple request/response".
This is an optional spec in the sense that no implementation is required to implement an API for this, but when I talk about a simple request/response that can make use of all libp2p transports this is what I mean. It would be helpful to agree on that part.