feat(transport): use parallel limited dialer #195

dignifiedquire · 2017-03-27T11:31:14Z

cc @jackkleeman @dryajov @diasdavid

I haven't added specific test cases, if you have some please drop them in the comments so I can add them.

Needs

Replaces #193

Fixes #194

Replaces #193 Fixes #194

daviddias · 2017-03-27T12:15:53Z

src/transport.js

+            }
+            if (q.canceled) {
+              log('dial canceled: %s', multiaddr.toString())
+              // clean up already done dials


Where is the conn being closed, if we already had dialed one?

hahahaha I forgot to write it

probably should use .close instead

yeah, https://github.com/libp2p/js-libp2p-swarm/pull/195/files#r108174470

jackkleeman · 2017-03-27T13:30:44Z

src/transport.js

+      } else {
+        log('setting up new queue')
+        q = queue((multiaddr, cb) => {
+          const conn = t.dial(multiaddr, (err) => {


does dial return an error to its callback? It doesn't seem to for the websocket transport

fixing this

I think I am finding a bug where the onConnect callback is being called even when the websocket 'open' event isn't called. Will file an issue. EDIT - actually scratch that, the problem is the websocket transport isn't handling errors correctly.

See libp2p/js-libp2p-websockets#60
After this PR, websockets will indeed return an error to its callback, so your code will work great 👍

Okay I got everything working, except that we need to upgrade both libp2p-tcp and libpp2-websocket to actually provide an error if one happened on the callback, otherwise the detection is horrific

No there wasn't a pull-ws issue after all, just a libp2p-websockets error handling issue
and I've issued a PR to libp2p-websockets, see my comment above

well it is both I am afraid, I actually can't detect the error in libp2p-websockets easily as the connect callback is called even though an error occured :(

I think onConnect is meant to be called with errors as the first argument. Why is that difficult to check for?

I opened then closed an issue on this as I realised that they sent errors to onConnect in several places. pull-stream/pull-ws#18

Just merged you pr, thank you it works great with that :)

daviddias · 2017-03-27T13:56:40Z

src/transport.js

+            if (q.canceled) {
+              log('dial canceled: %s', multiaddr.toString())
+              // clean up already done dials
+              pull(pull.empty(), conn)


The other end will try to write to this conn as soon as it opens and since we are not reading those values, it will stay forever open.

Use .close/destroy and make sure it gets destroyed (we don't want to exhaust open sockets/fd limits)

yeah, working on it

dignifiedquire · 2017-03-27T14:56:08Z

I've also added a timeout on the actual dial call so we don't hang indefinitely

daviddias · 2017-03-27T15:55:12Z

Seems that tests in swarm didn't catch all the cases, specially when there is WS + TCP involved. Getting these at libp2p-ipfs-nodejs:

 1) libp2p-ipfs-nodejs libp2p.dial using PeerInfo nodeA to nodeB:
     Uncaught TypeError: conn.close is not a function
      at dialWithTimeout (node_modules/libp2p-swarm/src/transport.js:70:22)
      at injectedCallback (node_modules/async/timeout.js:62:30)
      at transport.dial (node_modules/libp2p-swarm/src/transport.js:199:7)
      at f (node_modules/once/once.js:25:25)
      at Socket.rawSocket.once (node_modules/libp2p-tcp/src/index.js:40:7)
      at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1074:10)

TCP is missing the conn.close https://github.com/libp2p/js-libp2p-tcp/blob/master/src/index.js and also, WebRTC star doesn't have such a method https://github.com/libp2p/js-libp2p-webrtc-star/blob/master/src/index.js

Seems that a .close never really made it to the interface-connection (https://github.com/libp2p/interface-connection/blob/master/src/connection.js) and with good reason, it has a method to 'end' the call by sending the 'FIN' packet which gets fired once we do something like pull.empty(), needing only a 'destroy' event that would trickly to the socket.

daviddias · 2017-03-27T16:03:43Z

Will finish the update on the new libp2p API for bitswap and js-ipfs and come back to this after that :)

dignifiedquire · 2017-03-27T16:09:38Z

@diasdavid so should I simply change it to what I had before? pull(pull.empty(), conn)?

daviddias · 2017-03-27T17:15:05Z

Not quite the ideal solution, it has worked for us because we always end muxed streams and only close sockets when they get disconnected. Now we actually need to 'destroy' the socket.

* feat(transport): use parallel limited dialer

feat(transport): use parallel limited dialer

b04e954

Replaces #193 Fixes #194

dignifiedquire added the in progress label Mar 27, 2017

dignifiedquire requested a review from daviddias March 27, 2017 11:31

remove yarn.lock

5ee81ed

dignifiedquire mentioned this pull request Mar 27, 2017

Using the best address available ipfs/js-ipfs#812

Closed

daviddias reviewed Mar 27, 2017

View reviewed changes

actually cleanup connections

dfc9078

jackkleeman mentioned this pull request Mar 27, 2017

Attempt to dial all addresses in parallel and use the first success #193

Closed

jackkleeman reviewed Mar 27, 2017

View reviewed changes

daviddias reviewed Mar 27, 2017

View reviewed changes

fixes

76c20d6

jackkleeman approved these changes Mar 27, 2017

View reviewed changes

dignifiedquire mentioned this pull request Mar 27, 2017

fix(dial): proper error handling on dial libp2p/js-libp2p-tcp#77

Merged

daviddias added 2 commits March 27, 2017 16:26

chore: update deps

634ab78

chore: rm unused console.log

4f555c8

daviddias merged commit a15e63c into master Mar 27, 2017

daviddias deleted the feat/dial branch March 27, 2017 15:33

daviddias removed the in progress label Mar 27, 2017

daviddias mentioned this pull request Mar 27, 2017

Waiting for parallel swarm fix ipfs-inactive/js-libp2p-ipfs-nodejs#87

Closed

dignifiedquire mentioned this pull request Mar 27, 2017

fix(transport): use pull.empty for closing #200

Closed

dryajov pushed a commit that referenced this pull request Mar 29, 2017

feat(transport): use parallel limited dialer (#195)

2abd8d8

* feat(transport): use parallel limited dialer

dryajov pushed a commit that referenced this pull request Apr 2, 2017

feat(transport): use parallel limited dialer (#195)

9bbe1bd

* feat(transport): use parallel limited dialer

dryajov pushed a commit that referenced this pull request Apr 2, 2017

feat(transport): use parallel limited dialer (#195)

b4f3045

* feat(transport): use parallel limited dialer

dryajov pushed a commit that referenced this pull request Apr 2, 2017

feat(transport): use parallel limited dialer (#195)

fd8f237

* feat(transport): use parallel limited dialer

dryajov pushed a commit that referenced this pull request Apr 3, 2017

feat(transport): use parallel limited dialer (#195)

c2dcde3

* feat(transport): use parallel limited dialer

dryajov pushed a commit that referenced this pull request Apr 7, 2017

feat(transport): use parallel limited dialer (#195)

c7da761

* feat(transport): use parallel limited dialer

dryajov pushed a commit that referenced this pull request Apr 7, 2017

feat(transport): use parallel limited dialer (#195)

ac02d7a

* feat(transport): use parallel limited dialer

dryajov pushed a commit that referenced this pull request Apr 7, 2017

feat(transport): use parallel limited dialer (#195)

655f41c

* feat(transport): use parallel limited dialer

dryajov pushed a commit that referenced this pull request Apr 7, 2017

feat(transport): use parallel limited dialer (#195)

ccbf0db

* feat(transport): use parallel limited dialer

dryajov pushed a commit that referenced this pull request Apr 7, 2017

feat(transport): use parallel limited dialer (#195)

414cc98

* feat(transport): use parallel limited dialer

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(transport): use parallel limited dialer #195

feat(transport): use parallel limited dialer #195

dignifiedquire commented Mar 27, 2017 •

edited

Loading

daviddias Mar 27, 2017

dignifiedquire Mar 27, 2017

dignifiedquire Mar 27, 2017

dignifiedquire Mar 27, 2017

daviddias Mar 27, 2017

jackkleeman Mar 27, 2017

dignifiedquire Mar 27, 2017

jackkleeman Mar 27, 2017 •

edited

Loading

jackkleeman Mar 27, 2017 •

edited

Loading

dignifiedquire Mar 27, 2017

jackkleeman Mar 27, 2017 •

edited

Loading

dignifiedquire Mar 27, 2017 •

edited

Loading

jackkleeman Mar 27, 2017

jackkleeman Mar 27, 2017

dignifiedquire Mar 27, 2017

daviddias Mar 27, 2017

dignifiedquire Mar 27, 2017

dignifiedquire commented Mar 27, 2017

daviddias commented Mar 27, 2017 •

edited

Loading

daviddias commented Mar 27, 2017

dignifiedquire commented Mar 27, 2017

daviddias commented Mar 27, 2017

feat(transport): use parallel limited dialer #195

feat(transport): use parallel limited dialer #195

Conversation

dignifiedquire commented Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackkleeman Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

jackkleeman Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackkleeman Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

dignifiedquire Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dignifiedquire commented Mar 27, 2017

daviddias commented Mar 27, 2017 • edited Loading

daviddias commented Mar 27, 2017

dignifiedquire commented Mar 27, 2017

daviddias commented Mar 27, 2017

dignifiedquire commented Mar 27, 2017 •

edited

Loading

jackkleeman Mar 27, 2017 •

edited

Loading

jackkleeman Mar 27, 2017 •

edited

Loading

jackkleeman Mar 27, 2017 •

edited

Loading

dignifiedquire Mar 27, 2017 •

edited

Loading

daviddias commented Mar 27, 2017 •

edited

Loading