swarm/src/connection: Test max_negotiating_inbound_streams #2785
Conversation
Test that `HandlerWrapper` upholds the provided `max_negotiating_inbound_streams` limit.
|
@thomaseizinger @elenaf9 could one of you give this pull request a review? |
| use std::num::NonZeroU8; | ||
| use std::sync::Arc; | ||
|
|
||
| struct DummySubstream(Arc<()>); |
There was a problem hiding this comment.
| struct DummySubstream(Arc<()>); | |
| struct PendingSubstream(Arc<()>); |
There was a problem hiding this comment.
Plus, I'd prefer such kind of infrastructure to be at the bottom of the module. More generally, items in a module / file should be sorted by priority / likelihood of wanting to see them as you open the file and read it top to bottom.
Having imports at the top somewhat contradicts this rule but I think that is too controversial to adopt :D
| ); | ||
| } | ||
|
|
||
| QuickCheck::new().quickcheck(prop as fn(_)); |
There was a problem hiding this comment.
Have you ever considered using the #[quickcheck] macro?
There was a problem hiding this comment.
I haven't. Looks handy to me. Though I would prefer being consistent, i.e. move the entire code base to #[quickcheck] at once instead of diverging on a couple of pull requests.
| /// Implementation of [`UpgradeInfo`], [`InboundUpgrade`] and [`OutboundUpgrade`] that always | ||
| /// returns a pending upgrade. | ||
| #[derive(Debug, Copy, Clone)] | ||
| pub struct PendingUpgrade<P> { |
There was a problem hiding this comment.
Why do we need this to be generic?
There was a problem hiding this comment.
Are you suggesting to use a String instead? What if a user wants to use it with a type other than String that implements ProtocolName?
There was a problem hiding this comment.
With "user" I mean am thinking of potential users beyond the test introduced in this pull request.
There was a problem hiding this comment.
Are you suggesting to use a
Stringinstead? What if a user wants to use it with a type other thanStringthat implementsProtocolName?
&'static str would probably be my preference and I'd say we can make it generic once needed? For tests, a static protocol name is probably good enough for many if not all cases.
There was a problem hiding this comment.
libp2p-core has the ProtocolName abstraction. Thus I would suggest all of libp2p-core should use that abstraction. I am happy to design an alternative approach. Though I think that should happen in a different pull request.
|
Thanks for the review @thomaseizinger. I addressed all your comments. Let me know what you think. |
…est-max-negotiating
…est-max-negotiating
| _info: Self::OutboundOpenInfo, | ||
| ) { | ||
| void::unreachable(protocol); | ||
| #[allow(unreachable_code)] | ||
| { | ||
| void::unreachable(_info); | ||
| } |
There was a problem hiding this comment.
| _info: Self::OutboundOpenInfo, | |
| ) { | |
| void::unreachable(protocol); | |
| #[allow(unreachable_code)] | |
| { | |
| void::unreachable(_info); | |
| } | |
| _: Self::OutboundOpenInfo, | |
| ) { | |
| void::unreachable(protocol); |
I'd almost suggest to not use void::unreachable for the info part if we have to use #[allow(unreachable_code)] for it to work.
There was a problem hiding this comment.
I prefer the additional void::unreachable, just in case we refactor this part of the codebase in the future.
|
Thanks for the help @thomaseizinger! |
Description
Test that
HandlerWrapperupholds the providedmax_negotiating_inbound_streamslimit.Links to any relevant issues
Feature introduced in #2697.
Open Questions
Change checklist
[ ] I have made corresponding changes to the documentation[ ] I have added tests that prove my fix is effective or that my feature works[ ] A changelog entry has been made in the appropriate crates