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

Should ProtocolsHandler get a InboundOpenInfo type? #1709

Closed
twittner opened this issue Aug 17, 2020 · 5 comments
Closed

Should ProtocolsHandler get a InboundOpenInfo type? #1709

twittner opened this issue Aug 17, 2020 · 5 comments

Comments

@twittner
Copy link
Contributor

When libp2p-request-response was introduced, it used a combination of upgrades and channels which was seen as a "natural continuation" of the original approach to negotiate protocols. Timeout handling in the context of this approach proved to be difficult (see #1706) because identifying upgrades and upgrade errors is not always possible for inbound upgrades. It seems that the ProtocolsHandler trait is missing a type InboundOpenInfo which would be available in upgrade callbacks, mirroring the already existing OutboundOpenInfo type. This ticket is about the question if others see the need for such a type or not?

@twittner twittner changed the title Should ProtocolsHandler::InboundOpenInfo get InboundOpenInfo type? Should ProtocolsHandler::InboundOpenInfo get a InboundOpenInfo type? Aug 17, 2020
@tomaka
Copy link
Member

tomaka commented Aug 17, 2020

I know this isn't really documented, but in my mind it would be the other way around: the OutboundOpenInfo would be deprecated.

Since the user is free to pass any "meta-data" inside of the OutboundProtocol (and InboundProtocol) itself, the OutboundOpenInfo is a redundant mechanism.

@twittner
Copy link
Contributor Author

twittner commented Aug 17, 2020

This only applies to successful upgrades and does not help with upgrade errors, no?

Edit 1: For example timeouts.
Edit 2: Sloppy wording. What I meant was that it does not generally help with inject_{dial, listen}_upgrade_err because of ProtocolsHandlerUpgradeErrs such as Timeout or Timer.

@twittner twittner changed the title Should ProtocolsHandler::InboundOpenInfo get a InboundOpenInfo type? Should ProtocolsHandler get a InboundOpenInfo type? Aug 17, 2020
@romanb
Copy link
Contributor

romanb commented Aug 18, 2020

I don't see another or better approach to attach context information to inbound upgrades that is also available in the error case. Adding to that the fact that the multi handler cannot even be implemented correctly without such context (as seen in #1710) it seems rather important. libp2p-request-response is relying on inject_listen_upgrade_error to be called properly, otherwise all inbound request failures are silent. While this doesn't have immediate fatal consequences (i.e. memory leaks or anything like it), it is still very undesirable, not just for Throttled, since, if I remember correctly, paritytech/substrate#6634 even uses the multi handler at the moment. So I'm 👍 for InboundOpenInfo.

@twittner
Copy link
Contributor Author

twittner commented Aug 18, 2020

Another implication of {In,Out}boundUpgradeExt::map_{in, out}bound and {In, Out}boundUpgradeExt::map_{in, out}bound_err is that they usually create closure types which in turn require boxing in order to be named as associated ProtocolsHandler::{In, Out}boundProtocols.

@romanb
Copy link
Contributor

romanb commented Aug 24, 2020

Closed by #1714.

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

3 participants