A few BUGFIXES and new tests to cover those scenarios #54

michaelfakhri · 2016-12-13T07:10:53Z

Here is a summary of all the changes:

Variable maSelf was redirecting all dials to come back through the last added listener. This definitely does not make any sense, since the last added listener will most likely be another signalling server which will have a completely different multi-address. This means that the peer will never get the answer for the offer creating an error state on the server side and on the peer's side.
Closing the listener did not close the socket connection to the signalling server. This safe to do at the moment since each listener creates a new socket connection to be used.
Moved the close and getObservedAddr functions to be added only when the listen function is called since they do not make any sense if the listener is not listening.There is no multi-address to return or a listener to close if its not listening.
Fixed ss-leave event which did not pass the multi-address to the server
Fixed an issue where the listener was added with a function as the key to the listeners map/array
Moved adding the listener to the listeners array to occur only once its connected to the signalling server, otherwise its just have a bad listener that cannot actually dial anywhere

Part of this PR addresses Websocket stays connected even after webrtc-star listener is closed #51

EDIT: This PR also adds support for handling multiple signalling servers
Let me know if you need me to make any changes.

daviddias

Awesome PR @michaelfakhri, thank you for adding tests in. Made a couple of comments, would be great to have those addressed for the merge

daviddias · 2016-12-14T10:37:55Z

src/webrtc-star/index.js

+      listener.close = (callback) => {
+        if (!callback) {
+          callback = function noop () {}
+        }


let's replace this by a more simpler: callback = callback || () => {}

daviddias · 2016-12-14T10:38:41Z

src/webrtc-star/index.js

+        }, 100)
+      }
+      listener.getAddrs = (callback) => {
+        process.nextTick(() => {


This is not available in the browser, should use setImmediate instead

daviddias · 2016-12-14T10:39:20Z

test/webrtc-star/test-dial.js

@@ -71,3 +71,74 @@ describe('dial', () => {
    // TODO IPv6 not supported yet
  })
 })
+describe('complex dial scenarios', () => {
+  let ws1, ws2, listenerToClose


Are you using standard for linting? This fails linting

yes, everything was good using gulp lint on my machine and on travis. I used npm run lint the second time round and it was still fine, should I have used something else?

daviddias · 2016-12-14T10:43:03Z

src/webrtc-star/index.js

@@ -58,7 +57,7 @@ function WebRTCStar () {
    channel.on('signal', function (signal) {
      sioClient.emit('ss-handshake', {
        intentId: intentId,
-        srcMultiaddr: maSelf.toString(),
+        srcMultiaddr: listeners[Object.keys(listeners)[0]].ma.toString(),


This will make it fail if more than one listener is added. Can you add a test for multiple listeners?

I also added two tests earlier that test multiple listeners connected to the same signalling server but are listening on different addresses. The code here simply follows the same criteria for picking the sioClient. Nothing weird going on but the code previously attached a random address to the signalling which is weird. In a multiple signalling server scenario, that maSelf code could fail on server and client side.

daviddias · 2016-12-14T10:46:11Z

src/webrtc-star/index.js

@@ -116,11 +115,11 @@ function WebRTCStar () {
      if (!callback) {
        callback = function noop () {}
      }


let's replace this by a more simpler: callback = callback || () => {}

michaelfakhri · 2016-12-14T13:18:28Z

src/webrtc-star/index.js

-    if (!callback) {
-      callback = function noop () {}
-    }
+    callback = callback || function () {}

    const intentId = (~~(Math.random() * 1e9)).toString(36) + Date.now()
    const sioClient = listeners[Object.keys(listeners)[0]].io


@diasdavid I think the criteria for picking the sioClient here is not quite right, since it should be picking the signalling that we know the other peer is connected to (from the ma). This method just picks the first signalling server available and sends the offer without taking into account that peer might not be connected to that specific server. (Keeping in mind this only a problem in multiple signalling server scenario)

You are right @michaelfakhri, the picking of the 'right signalling server' wasn't added because we always had only one, but we should support multiple nevertheless.

Would you like to add that + a test?

Yeah sure, perhaps I might also attempt to decouple the listeners from the signalling servers in the process.

michaelfakhri · 2017-01-15T22:04:25Z

note to self 1: simple-peer 6.1.3 breaks libp2p webrtc transport (something to do with onSuccess callback), 6.1.0 work just fine so I used npm shrinkwrap to get tests passing by forcing simple peer to 6.1.0
note to self 2: df2 fix callback error in tests

daviddias · 2017-01-16T04:36:32Z

Interesting note, thanks for catching that @michaelfakhri, will check how to update properly :)

daviddias · 2017-01-16T13:41:55Z

@michaelfakhri found the issue, the 'Fallback' option in simple-peer is expecting an interface that wrtc doesn't support - feross/simple-peer@e9ffcfa#commitcomment-20490182

daviddias · 2017-01-16T13:45:59Z

@michaelfakhri could you rebase master onto your PR? I've fixed the simple-peer version while it gets resolved, no need for shrinkwrap right now :)

https://greenkeeper.io/

michaelfakhri · 2017-01-17T12:13:57Z

@diasdavid Thanks for looking into it and fixing it!

This is now ready for review / merge.

I attempted a rebase but did something wrong and ended up bringing in all the commits into the branch instead of rebasing but thats easily fixable using a squash merge at the end.

I have seen two issues (not related to this PR though) that might need attention / investigation:

TRAVIS CI firefox browser tests are not all running (for e.g: dial is not run and it seems to be just hanging)
TRAVIS CI karma test crashes with segmentation fault on node.js v4 on master and this PR too

michaelfakhri · 2017-01-26T06:20:51Z

Continued here: #89

daviddias suggested changes Dec 14, 2016

View reviewed changes

michaelfakhri commented Dec 14, 2016

View reviewed changes

michaelfakhri changed the title ~~A few BUGFIXES and new tests to cover those scenarios~~ WIP: A few BUGFIXES and new tests to cover those scenarios Dec 20, 2016

michaelfakhri added 8 commits January 15, 2017 15:11

A few BUGFIXES and new tests to cover those scenarios

33b18f3

some of the requested changes

d4ea43e

lint error

1ed3727

A few BUGFIXES and new tests to cover those scenarios

f11a78d

some of the requested changes

e5592fd

lint error

c391214

sample test

a39f3d9

New test and multiple sig server handling

c11a2e1

michaelfakhri force-pushed the master branch from 9fe9ca1 to 1ed3727 Compare January 15, 2017 20:14

michaelfakhri added 4 commits January 15, 2017 15:25

New test and multipl sig server

39fcefc

include merge of test-dial.js - missed it first time

4c322a4

temp shrink-wrap for testing

ddecc91

revert to older simple-peer to avoid onSuccess error

e053355

daviddias and others added 10 commits January 17, 2017 06:15

chore: update deps and fix simple-peer to 6.1.0

4304663

chore: update contributors

f0afad6

chore: release version v0.7.1

75dcddf

chore(package): update mafmt to version 2.1.3

8d69cf8

https://greenkeeper.io/

chore(package): update mafmt to version 2.1.4

d82b431

https://greenkeeper.io/

chore: release version v0.7.2

c264d1a

A few BUGFIXES and new tests to cover those scenarios

5cf4c78

avoid stopping sig server to avoid callback err

c1b8cc8

Merge branch 'master' into master

8773c73

remove extra line added during rebase

33a302a

michaelfakhri changed the title ~~WIP: A few BUGFIXES and new tests to cover those scenarios~~ A few BUGFIXES and new tests to cover those scenarios Jan 17, 2017

michaelfakhri mentioned this pull request Jan 26, 2017

A few BUGFIXES and new tests to cover those scenarios + multiple signalling servers! #89

Closed

michaelfakhri closed this Jan 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A few BUGFIXES and new tests to cover those scenarios #54

A few BUGFIXES and new tests to cover those scenarios #54

michaelfakhri commented Dec 13, 2016 •

edited

Loading

daviddias left a comment

daviddias Dec 14, 2016

michaelfakhri Dec 14, 2016

daviddias Dec 14, 2016

michaelfakhri Dec 14, 2016

daviddias Dec 14, 2016

michaelfakhri Dec 14, 2016 •

edited

Loading

daviddias Dec 14, 2016

michaelfakhri Dec 14, 2016

daviddias Dec 14, 2016

michaelfakhri Dec 14, 2016

michaelfakhri Dec 14, 2016 •

edited

Loading

daviddias Dec 18, 2016

michaelfakhri Dec 20, 2016

michaelfakhri commented Jan 15, 2017

daviddias commented Jan 16, 2017

daviddias commented Jan 16, 2017

daviddias commented Jan 16, 2017

michaelfakhri commented Jan 17, 2017

michaelfakhri commented Jan 26, 2017

A few BUGFIXES and new tests to cover those scenarios #54

A few BUGFIXES and new tests to cover those scenarios #54

Conversation

michaelfakhri commented Dec 13, 2016 • edited Loading

daviddias left a 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

michaelfakhri Dec 14, 2016 • 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

michaelfakhri Dec 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelfakhri commented Jan 15, 2017

daviddias commented Jan 16, 2017

daviddias commented Jan 16, 2017

daviddias commented Jan 16, 2017

michaelfakhri commented Jan 17, 2017

michaelfakhri commented Jan 26, 2017

michaelfakhri commented Dec 13, 2016 •

edited

Loading

michaelfakhri Dec 14, 2016 •

edited

Loading

michaelfakhri Dec 14, 2016 •

edited

Loading