multiformats / go-multistream

an implementation of the multistream protocol in go
MIT License
41 stars 27 forks source link

[RFC] remove handler func support #19

Open Stebalien opened 6 years ago

Stebalien commented 6 years ago
  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 c9587f16af873eced8fd29ea66c9c777d5c35d09

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).

Stebalien commented 6 years ago

@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 commented 6 years ago

Coverage Status

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

Stebalien commented 6 years ago

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.

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 commented 6 years ago

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.

😭