Skip to content
This repository has been archived by the owner on Sep 6, 2022. It is now read-only.

Interface to verify if a peer supports a protocol without making allocations #148

Merged
merged 3 commits into from
May 14, 2020

Conversation

aarshkshah1992
Copy link
Contributor

@aarshkshah1992 aarshkshah1992 commented May 8, 2020

Motivation: libp2p/go-libp2p-kad-dht#594.

In a lot of cases, we simply want to know if a peer supports any one protocol among a set of protocols.
In such cases, we currently end up doing:

if s, err := as.host.Peerstore().SupportsProtocols(peer, protos...); 
len(s) == 0 && err == nil {...}

However, this causes needless allocations as SupportsProtocols makes an allocation for each protocol that is supported even if we don't actually need that information.

This PR adds a SupportsAnyProtocol API that allows clients to determine if the peer supports ANY of the given protocols without knowing which of the given protocols it actually supports.

@aarshkshah1992 aarshkshah1992 changed the title Check if peer supports a protocol without allocation Interface to verify if a peer supports a protocol without making allocations May 8, 2020
Stebalien
Stebalien previously approved these changes May 8, 2020

// SupportsAnyProtocol returns true if the peer supports ATLEAST ONE of the given protocols and false otherwise.
// If the returned error is not nil, the result is indeterminate.
SupportsAnyProtocol(peer.ID, ...string) (bool, error)
Copy link
Member

Choose a reason for hiding this comment

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

What about FirstSupportedProtocol(peer.ID, ...string) (string, error)? This is what you want 99.9% of the time anyways and covers the new stream use-case.

Copy link
Contributor Author

@aarshkshah1992 aarshkshah1992 May 11, 2020

Choose a reason for hiding this comment

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

@Stebalien Great idea ! This is done.

@Stebalien Stebalien dismissed their stale review May 8, 2020 20:06

(was supposed to be a comment)

@aarshkshah1992
Copy link
Contributor Author

@Stebalien Have made the interface change here and in the peerstore PR. Please take a look when you can. Thanks !

@Stebalien
Copy link
Member

Merge and cut a minor release when ready.

@aarshkshah1992 aarshkshah1992 merged commit 1c39960 into master May 14, 2020
@raulk raulk deleted the feat/supports-alloc branch May 14, 2020 15:01
@Stebalien Stebalien mentioned this pull request May 26, 2020
77 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants