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

Remove the OutboundOpenInfo associated type from ProtocolsHandler #696

Closed
tomaka opened this issue Nov 28, 2018 · 3 comments
Closed

Remove the OutboundOpenInfo associated type from ProtocolsHandler #696

tomaka opened this issue Nov 28, 2018 · 3 comments

Comments

@tomaka
Copy link
Member

tomaka commented Nov 28, 2018

(Long term issue. Let's wait for things to be stable before seeing if that's a good idea.)

Initially, the ProtocolsHandler contains the associated types Upgrade and OutboundOpenInfo. The Upgrade was used both for inbound and outbound substreams.
The OutboundOpenInfo is an additional piece of data that you can pass when you open an outbound substream, and that you get back later, in order to remember why you opened this substream.

However now that the Upgrade has been split into InboundUpgrade and OutboundUpgrade, the user can insert this piece of information in the OutboundUpgrade instead.

@mzabaluev
Copy link
Contributor

There is an asymmetry that may be addressed by OutboundOpenInfo, probably after some refactoring: the state for outbound upgrade typically carries data to start the outbound communication with, such as a protocol request in OneShotHandler or RequestResponseHandler. For inbound upgrades, the protocol handler has to start off with only local state.

This shows up in the current implementations of request-oriented protocols, where UpgradeInfo is implemented separately for each direction without a good reason for it (the implementations are mostly identical). It would be better if, as the general case, InboundUpgrade and OutboundUpgrade could be implemented for the same protocol state type and Self::OutboundOpenInfo carried protocol data for the initial outbound communication.

@mzabaluev
Copy link
Contributor

The current API does not seem to be streamlined for request-response protocols: the output type of OutboundUpgrade is most naturally treated as the protocol response, so the request data tend to be applied in the upgrade future that actually performs the request or the whole request-response roundtrip. Ditto for InboundUpgrade: the output "wants" to be a fully decoded request. Hence the "need" for InboundUpgrade and OutboundUpgrade to be implemented on different types. An alternative is to only do streaming state set up in InboundUpgrade/OutboundUpgrade and defer request-response I/O to be triggered by ProtocolsHandler::inject_fully_negotiated_*, but this increases state handling complexity inside the ProtocolsHandler impl.

@romanb
Copy link
Contributor

romanb commented Aug 25, 2020

As per #1714 (ticket #1709) not only did we not remove OutboundOpenInfo but we added a symmetrical InboundOpenInfo, the reason being that these associated types are the only way to retain context information for an outbound or inbound substream that is always available to the ProtocolsHandler, whether the upgrade succeeds or fails, and even in the presence of failures occurring outside the scope of upgrade_outbound and upgrade_inbound, e.g. timeouts.

The current API does not seem to be streamlined for request-response protocols: the output type of OutboundUpgrade is most naturally treated as the protocol response, [..]. Ditto for InboundUpgrade: the output "wants" to be a fully decoded request.

Yes, implementing request-response protocols with the current APIs leaves something to be desired, which is why we wrote libp2p-request-response. The primary, generic design principle of InboundUpgrade::upgrade_inbound and OutboundUpgrade::upgrade_outbound is to do the I/O that is necessary to prepare a protocol for use, e.g. a handshake of some sort. multistream-select negotiates which protocol to use and then the upgrade "upgrades" the I/O stream to that protocol. If the protocol involves no handshake, strictly speaking there is nothing to do in upgrade_inbound or upgrade_outbound and reading or writing requests or responses should be left to the ProtocolsHandler. It's just that it happens to be so convenient, especially for request-response protocols with one substream per request to do the actual request/response I/O right there in the upgrades, as is done under the hood in libp2p-request-response.

@mzabaluev If you have suggestions for improving the API, please open new issues. All feedback and suggestions are appreciated.

@romanb romanb closed this as completed Aug 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants