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

[pubsub] Orbit electron concerns -- clashing ports and repos #104

Closed
jbenet opened this issue Sep 13, 2016 · 12 comments
Closed

[pubsub] Orbit electron concerns -- clashing ports and repos #104

jbenet opened this issue Sep 13, 2016 · 12 comments

Comments

@jbenet
Copy link

jbenet commented Sep 13, 2016

Status quo

current state of orbit-electron@pubsub, using go-ipfs@feat/pubsub

  • users may have installations of go-ipfs. at any of {@0.4.2 or lower, @0.4.3-rc#, @0.4.4-dev, @feat/pubsub}
  • users may have lingering $user/.ipfs repos at various versions. {1, 2, 3, 4}
  • orbit-electron requires using local repo ($user/.ipfs)
  • orbit-electron may use local ipfs or its own (i believe as of today it uses its own, go-ipfs@feat/pubsub}
  • if orbit-electron fails to use the repo (wrong version, locked, etc), the user gets no feedback, only a white screen on startup.
  • if orbit-electron fails to use the daemon, the user gets no feedback (? not sure about this)
  • if orbit-electron go-ipfs daemon fails to bind ports, it will fail, but orbit may still work and use the wrong daemon (no feedback)
  • if orbit-electron uses the daemon but it does not support pubsub, user gets no feedback, but things won't work well (? not sure about this)

Which add up to

  • failures on startup, which are not fixable by the user (without help from other people or github repositories. not good)
  • failures on startup that make it difficult for people to "pass an application bundle (.app) to each other, double click, and enjoy".
  • failures with terrible UX ("white screen", silent failures, etc)

Why all this is big problem

  • it is a goal for devcon that others are able to try our stuff on the spot
  • it is a goal for devcon that others are impressed by our tech and want to work with it / build on it
  • users may have installations of ipfs
  • users may want to try orbit
  • users will have broken UX and not know easily how to fix it
  • even if users manage to figure it out, they will react like "ughhh wtf it should be smarter than this"
  • this makes orbit almost entirely unusable by people who are not us.
  • I would not feel comfortable letting people try it

Suggested fixes

Fix1: No clashes (easiest)

Change Orbit to ensure it NEVER clashes with user or system ipfs processes, nor "other people's servers at ports :8080, :5001, :4001.

This means:

  • Orbit ALWAYS uses its own go-ipfs binary
  • Orbit chooses its own ports for ipfs daemon and sets them in the ipfs config
    • either by choosing other ports (maybe :8180, :5101, :4101) -- this may still clash with other services, so orbit SHOULD STILL show an error if ipfs daemon fails to bind
    • or by ensuring there are no port conflicts (use /tcp/0 in the ipfs config addresses, and read the bound ports from the daemon output)
  • Orbit ALWAYS uses its own local repo
  • Orbit chooses its own repo address.
    • either within the app bundle (flaky and prone to failure if people give each other the .app)
    • or at some well-known system location (eg $user/Application\ Support on OSX)

Pros:

  • no weird failures
  • easiest to do

Cons:

  • does not use people's local ipfs repos for content (note: not a problem if they user's ipfs daemon is running, because in that case Orbit's ipfs node will connect to user's ipfs node and get the content locally.

Fix2: User Friendly (harder)

  • Be explicit about what API and what repo are being used by orbit.
    • Easiest way to do this is two fields to the Orbit splash screen.
      • one for Repo populated with /Users/myuser/.ipfs and a help ( ? ) that gives users a bit of necessary context for changing it.
      • one for IPFS API populated with /ip4/127.0.0.1/tcp/5001 and a help ( ? ) that gives users a bit of necessary context for changing it.
      • maybe a third field ipfs bin, a path to the ipfs binary used by Orbit, populated with /path/to/Orbit.app/path/to/ipfs-floodsub
  • Have proper error handling (no "white screen failure"), in the orbit splash screen that correctly diagnoses problems and correctly gives user solutions. Problems to diagnose include (but not limited to):
    • no daemon available, and failure in starting one
    • some port in use, not ipfs daemon
    • some port in use, wrong ipfs version (does not support pubsub)
    • no repo in right location, but failed to init
    • repo in right location, but wrong version
      • IMPORTANT: DO NOT migrate a repo for the user without asking!
    • repo in right location, right version, but locked?

Pros:

  • explicit in how to use it
  • can be recovered
  • no "non starter" failures

Cons:

  • more complex splash UX (may be ok for this build)
  • have to do very complex error handling + recovery
@victorb
Copy link

victorb commented Sep 13, 2016

@haadcode I'm happy to help out with these if we can make a bullet list and I'll take some of the tasks, assuming this is something that needs fixing before devcon @jbenet

@ghost
Copy link

ghost commented Sep 13, 2016

does not use people's local ipfs repos for content (note: not a problem if they user's ipfs daemon is running, because in that case Orbit's ipfs node will connect to user's ipfs node and get the content locally.

this can be achieved by unconditionally connecting to /ip4/127.0.0.1/tcp/4001 -- it'll always work, no matter the mDNS weirdnesses. (did we ever test mDNS on localhost?)

@haadcode
Copy link
Member

Fix2: User Friendly (harder)

We definitely have a different bar what is user friendly ;)

Valid points re. the demo-esque nature of this build. Making sure it works is not much work, mainly handling errors properly in Orbit.

Easiest way to do this is two fields to the Orbit splash screen.

Even though it's a demo, these kind of configurations don't belong in the UI. I'll see what's the best way and make it a fluid experience so that there's no user configuration required. I'll let you know when these are fixed.

@dignifiedquire
Copy link
Contributor

Re ports in use, you could also try different ports, if one fails to bind.

@haadcode
Copy link
Member

I made a commit that (latest pubsub branch):

  • Uses application specific IPFS data directory (Library/Application Support/orbit-floodsub-ipfs-repo)
  • Make sure to either pickup a running floodsub-0 daemon
  • Or ask the user to shutdown running (incompatible) daemons
  • Starts a floodsub-0 daemon if none running and binds to .../tcp/0

These steps make it behave nicely and while the user experience is not fantastic, it prevents errors, makes sure the user gets feedback and guides the user to remedy the situation.

For the future, to make this (controlling the daemon and error handling) nice, we'll have to fix things in ipfsd-ctl, there be dragons.

@haadcode
Copy link
Member

Improved the path and error handling further with this commit: a2256d5

@victorb
Copy link

victorb commented Sep 14, 2016

@haadcode not sure if it's accidental or not, but a2256d5 contains code outputted from build in client/dist/

@haadcode
Copy link
Member

not sure if it's accidental or not, but a2256d5 contains code outputted from build in client/dist/

Not accidental. Putting the UI build in so that it's quicker to get started (instead of doing 2x npm install & npm run build).

@jbenet
Copy link
Author

jbenet commented Sep 14, 2016

thanks for improving this 👍 ❤️

@haadcode
Copy link
Member

Improved this further as per our discussion: Orbit now always spawns its own daemon.

@jbenet
Copy link
Author

jbenet commented Sep 16, 2016

Thanks ❤️❤️❤️
On Fri, Sep 16, 2016 at 5:47 AM Haad notifications@github.com wrote:

Improved this further as per our discussion: Orbit now always spawns
its own daemon.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#104 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAIcoQLaO1Hbq15-87NF3M6Eppapc1Q9ks5qqmWpgaJpZM4J8C-k
.

@haadcode
Copy link
Member

I believe these issues were resolved during the time leading up to Devcon2. The fixes are in devcon2 branch and will be merged to master along with all the other improvements and new features. If you experience more "smoothness" issues, please open a new issue. Thank you all for the testing and work you did for this! <3

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants