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

Simple request/response #561

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

Simple request/response #561

wants to merge 9 commits into from

Conversation

MarcoPolo
Copy link
Contributor

@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.

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.

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.

simple-request-response/README.md Outdated Show resolved Hide resolved
simple-request-response/README.md Outdated Show resolved Hide resolved
simple-request-response/README.md Outdated Show resolved Hide resolved
simple-request-response/README.md Outdated Show resolved Hide resolved
simple-request-response/README.md Outdated Show resolved Hide resolved
@thomaseizinger
Copy link
Contributor

I'll review tomorrow.

@thomaseizinger
Copy link
Contributor

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.

This is the first time I am hearing about such a thing as an explicitly optional spec. I thought that is the default?

Copy link
Contributor

@thomaseizinger thomaseizinger left a 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 :)

simple-request-response/README.md Outdated Show resolved Hide resolved
simple-request-response/README.md Outdated Show resolved Hide resolved
simple-request-response/README.md Outdated Show resolved Hide resolved
simple-request-response/README.md Outdated Show resolved Hide resolved
simple-request-response/README.md Outdated Show resolved Hide resolved
simple-request-response/README.md Outdated Show resolved Hide resolved
simple-request-response/README.md Outdated Show resolved Hide resolved
@MarcoPolo MarcoPolo force-pushed the marco/simple-req-resp branch from d95cd25 to 1e1c96d Compare July 17, 2023 20:40
@MarcoPolo
Copy link
Contributor Author

MarcoPolo commented Jul 17, 2023

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.

@MarcoPolo MarcoPolo mentioned this pull request Jul 17, 2023
Copy link
Contributor

@thomaseizinger thomaseizinger left a 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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previous discussion: #561 (comment)

Comment on lines +75 to +76
(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
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Base automatically changed from http to master June 13, 2024 20:54
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.

5 participants