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

add conflict resolution to the multiplexer #31

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Stebalien
Copy link
Member

Motivation: As it stands, multiple, independent services trying to use the same libp2p swarm can clobber each-other's protocol handlers without noticing. This is fine when protocol registration is all centrally manged but can go horribly wrong in pluggable systems (e.g., the ipfs p2p commands and a theoretical libp2p daemon).

This patch:

  1. Removes AddHandlerFunc as it's impossible to tell if any two handlers registered this way conflict.
  2. Makes AddHandler return an error on conflict.
  3. Adds some additional options to AddHandler to help with conflict handling and cover the primary use-case of AddHandlerFunc

See the documentation on the options added in this patch for more details.


This shouldn't break anything except:

  1. Users that use AddHandlerFunc (i.e., nobody).
  2. Users that rely on AddHandler overriding existing protocol handlers (hopefully nobody).

This *shouldn't* break anything *except*:

1. Users that use `AddHandlerFunc` (i.e., nobody).
2. Users that rely on `AddHandler` overriding existing protocol
   handlers (hopefully nobody).
@Stebalien
Copy link
Member Author

@whyrusleeping I decided to turn off overriding by default as personally, that's what I'd expect.

@Stebalien
Copy link
Member Author

Thinking through this, this does have a significant downside: It makes it hard to negotiate an alternative protocol. Let's say I support /ipfs/bitswap/1.*.*, /ipfs/graphsync/1.*.* and my peer supports /ipfs/bitswap/2.1.1 and /ipfs/graphsync/1.9.0. Ideally, we'd negotiate /ipfs/graphsync/1.9.0 but, given prefix support only, I can't find a way to do this.

One solution is to add an additional Semver option that means "treat the final component as a version". That should still allow us to check for conflicts. Unfortunately, the implementation is looking a bit complicated. I'm going to see if we can pull the p2p patch through without fixing this (yet).

@Stebalien
Copy link
Member Author

Specifically, allowing one version constraint per protocol is easy. The tricky part is registering different handlers for different version ranges.

@magik6k
Copy link

magik6k commented Sep 4, 2018

I wonder what js does here

@raulk
Copy link
Member

raulk commented Sep 4, 2018

This kind of version-based protocol negotiation has proven important in devp2p (Ethereum), where the core protocol has evolved from eth62 to eth63 in the last years.

What if –internally– we make all protocols have a version? We can then fall back to a virtual 0.0.0 version at handler registration time when unversioned protocols are registered.

Version 0 would then implicitly the lowest precedence, so registering a /1.0.0 afterwards, would have the effect of the former handler managing [0.0.0, 1.0.0), and the latter handling [1.0.0, ∞), which, from the user standpoint, I'd argue is valid behaviour.

@raulk
Copy link
Member

raulk commented Sep 4, 2018

With the above approach, the protocol handler with higher specificity would win, if multiple were applicable. Just some thoughts ;-)

@Stebalien
Copy link
Member Author

We can do it, the code just gets tricker and I want to make sure we'd end up covering all cases we need. The code this replaces accepts an arbitrary function and is therefor maximally flexible. The issue is that, due to this flexibility, we can't detect conflicts. This isn't an issue now but we're trying to make this system extensible (by end users) so allowing conflict detection/resolution directly in the protocol handler would be nice.

Implicit versions is an interesting idea, we've been working on a replacement for this protocol but we'll need to keep versioning in bind for that. For some historical context, protocol names were supposed to be IPFS paths: /ipfs/Qm.../my-protocol/README.md (or something like that). In that world, semver doesn't really work, but, in practice, we never went this route anyways (protocol names were too long).

Personally, I'm not really convinced semver is the right way to go for protocols in a decentralized system regardless. My preference is a major version (or just change the protocol name) and feature detection/declaration. Semver was designed for an inherently centralized system (software publishing) and doesn't enable decentralized evolution (extension by multiple parties) very well.

@Stebalien
Copy link
Member Author

I wonder what js does here

They just use a function.

@magik6k magik6k mentioned this pull request Sep 5, 2018
5 tasks
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

Successfully merging this pull request may close these issues.

3 participants