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

[RFC] remove handler func support #19

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Stebalien
Copy link
Member

@Stebalien Stebalien commented Dec 29, 2017

  1. It never actually worked (we always used the text match).
  2. It made it impossible to accurately list protocols (required by the protocol).

reverts c9587f1

Alternatively (I'd actually prefer this) we should consider removing the "ls" command. That's not the job of this protocol (should use, e.g., an identify service).

1. It never actually worked (we always used the text match).
2. It made it impossible to list protocols.

reverts c9587f1
@Stebalien
Copy link
Member Author

@diasdavid thoughts? (FYI, by "it never worked", I mean "it never worked in go"). It looks like js-libp2p does support this but I'd still like to go one way or another. Either support listing supported protocols or don't support listing supported protocols.

@coveralls
Copy link

coveralls commented Dec 29, 2017

Coverage Status

Coverage decreased (-1.2%) to 78.272% when pulling cd4d405 on feat/remove-handler-func into b8f1996 on master.

@Stebalien
Copy link
Member Author

To make this clear, we have two conflicting features:

  1. We have ls to list all supported protocols.
  2. We allow registering dynamic protocol handlers.
  • The first feature implies that we must know all supported protocols up-front.
  • The second feature prevents us from knowing all protocols up-front.

There are three ways of fixing this:

  1. Removing the dynamic protocol feature (this PR).
  2. Remove the ls protocol.
  3. Redefine the ls protocol to only list known protocols.

I filed this PR because the dynamic protocol feature is currently broken in go (and has never worked).

@Stebalien
Copy link
Member Author

So... It turns out that the ls protocol is also broken in go.

  1. NegotiateLazy doesn't implement it (it "lists" no protocols).
  2. Negotiate writes back the protocol list but doesn't acknowledge the ls protocol first (that is, it just sends back the list of supported protocols without sending back <len>ls\n first.

😭

@Kubuxu Kubuxu self-requested a review February 24, 2018 22:35
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.

2 participants