core/upgrade/: Add ReadyUpgrade
#2855
Conversation
thomaseizinger
force-pushed
the
ready-upgrade
branch
from
2c580d9 to
6dc8817
Compare
|
I am planning to use this throughout the codebase where applicable. Should I do this as part of this PR or send separate ones @mxinden? It could for example be used in the ping protocol. |
There was a problem hiding this comment.
I am in favor of introducing a ReadyUpgrade.
Note that long term I would like to get rid of the upgrade mechanism in ConnectionHandler, though that still needs more thought.
I am planning to use this throughout the codebase where applicable. Should I do this as part of this PR or send separate ones @mxinden? It could for example be used in the ping protocol.
I think every pull request should be:
- As small as possible.
- Atomic.
- Leave
masterin the same or better state.
For the sake of the latter, concretely here for the sake of code deduplication, I think this pull request should include the changes across the many use-cases.
|
@mxinden The only place where it was directly applicable was the ping protocol. |
| impl UpgradeInfo for Ping { | ||
| type Info = &'static [u8]; | ||
| type InfoIter = iter::Once<Self::Info>; | ||
|
|
||
| fn protocol_info(&self) -> Self::InfoIter { | ||
| iter::once(PROTOCOL_NAME) | ||
| } | ||
| } | ||
|
|
||
| impl InboundUpgrade<NegotiatedSubstream> for Ping { | ||
| type Output = NegotiatedSubstream; | ||
| type Error = Void; | ||
| type Future = future::Ready<Result<Self::Output, Self::Error>>; | ||
|
|
||
| fn upgrade_inbound(self, stream: NegotiatedSubstream, _: Self::Info) -> Self::Future { | ||
| future::ok(stream) | ||
| } | ||
| } | ||
|
|
||
| impl OutboundUpgrade<NegotiatedSubstream> for Ping { | ||
| type Output = NegotiatedSubstream; | ||
| type Error = Void; | ||
| type Future = future::Ready<Result<Self::Output, Self::Error>>; | ||
|
|
||
| fn upgrade_outbound(self, stream: NegotiatedSubstream, _: Self::Info) -> Self::Future { | ||
| future::ok(stream) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Emphasizing this, though I think we are of the same opinion: While not de-duplicating given that ReadyUpgrade is only used once, I do think these kind of pull requests do a great job simplifying the business logic, here libp2p-ping, moving auxiliary not-directly-related logic to common places, here libp2p-core.
There was a problem hiding this comment.
Yes, strong agreement here.
|
Comments addressed, ready to merge from my end @mxinden! |
Description
This is the counterpart to
PendingUpgrade.Links to any relevant issues
Extracted out of #2852.
Open Questions
Change checklist
- [ ] I have added tests that prove my fix is effective or that my feature works