Timeout trigger _iceComplete if candidates seen.

[chr15m]

On some networks the iceStateChange=complete event will not fire until the browser times out, even though all valid and useful offer/answer candidates have already been gathered in the first milliseconds.

Examples of this occurring:

• In Improve initial performance for ws webtorrent/bittorrent-tracker#199 (45s delay)
• Slow ice candidate gathering onsip/SIP.js#162 (18s delay)
• https://bugs.chromium.org/p/webrtc/issues/detail?id=4699 (10s delay)

According to a Chromium developer here:

It's almost surely due to an interface that has an IP but no connectivity to the internet. We fail to get any response to our attempts to contact the STUN server, so we just time out.

In other words, on these networks there are valid routes to the internet but also invalid routes. Those invalid routes are confusing the offer gathering process into thinking it must keep trying.

This means that people on such networks can have 10s of seconds of delay in WebRTC offer creation time (consistently ~40s on my own network which experiences this issue) even though all valid candidates they are going to receive have been received in the first milliseconds of the offer process. The log in the bittorrent-tracker issue has a detailed example of this happening.

In the thread above Chromium developers suggest a JS time-out:

Seems like an application-specific decision.
...
Which is basically "use a timeout in JS", which is what I suggested

Similar patches have been suggested and implemented in other WebRTC projects:

• Slow ice candidate gathering onsip/SIP.js#162 (comment)
• https://stackoverflow.com/questions/28450738/onicegatheringstatechange-event-does-not-trigger-during-candidates-gathering/28457194#28457194
• onicecandidate handling node-webrtc/node-webrtc#44 (comment)

This patch follows the same pattern and makes _iceComplete fire early and return the valid offers that have been gathered after some timeout. This only happens once the first valid candidate has been received. The timeout defaults to 5 seconds and can be modified by the user using opts.iceCompleteTimeout.

Let me know if you need any further information or if there are any changes required to this patch. Thanks for your consideration!

Side note which may be of interest. On the network where I see the 45s delay the following simple-peer tests also fail:

# ensure remote address and port are available right after connection
ok 24 peers connected
ok 25 peer1 remote address is present
not ok 26 peer1 remote port is present
  ---
    operator: ok
    expected: true
    actual:   0
    at: Peer.<anonymous> (/home/chrism/dev/simple-peer/test/basic.js:213:7)
    stack: |-
      Error: peer1 remote port is present
          at Test.assert [as _assert] (/home/chrism/dev/simple-peer/node_modules/tape/lib/test.js:224:54)
          at Test.bound [as _assert] (/home/chrism/dev/simple-peer/node_modules/tape/lib/test.js:76:32)
          at Test.assert (/home/chrism/dev/simple-peer/node_modules/tape/lib/test.js:342:10)
          at Test.bound [as ok] (/home/chrism/dev/simple-peer/node_modules/tape/lib/test.js:76:32)
          at Peer.<anonymous> (/home/chrism/dev/simple-peer/test/basic.js:213:7)
          at emitNone (events.js:106:13)
          at Peer.emit (events.js:208:7)
          at /home/chrism/dev/simple-peer/index.js:772:12
          at /home/chrism/dev/simple-peer/index.js:635:7
          at <anonymous>
  ...
ok 27 peer2 remote address is present
not ok 28 peer2 remote port is present
  ---
    operator: ok
    expected: true
    actual:   0
    at: Peer.<anonymous> (/home/chrism/dev/simple-peer/test/basic.js:217:9)
    stack: |-
      Error: peer2 remote port is present
          at Test.assert [as _assert] (/home/chrism/dev/simple-peer/node_modules/tape/lib/test.js:224:54)
          at Test.bound [as _assert] (/home/chrism/dev/simple-peer/node_modules/tape/lib/test.js:76:32)
          at Test.assert (/home/chrism/dev/simple-peer/node_modules/tape/lib/test.js:342:10)
          at Test.bound [as ok] (/home/chrism/dev/simple-peer/node_modules/tape/lib/test.js:76:32)
          at Peer.<anonymous> (/home/chrism/dev/simple-peer/test/basic.js:217:9)
          at emitNone (events.js:106:13)
          at Peer.emit (events.js:208:7)
          at /home/chrism/dev/simple-peer/index.js:772:12
          at /home/chrism/dev/simple-peer/index.js:635:7
          at <anonymous>
  ...
ok 29 peer1 destroyed
ok 30 peer2 destroyed

[t-mullen]

So basically a timeout on ice gathering when trickle:false? Makes sense, I've noticed gathering some candidates can take a really long time, especially when using unresponsive STUN servers alongside active ones.

My only issue would be that ice candidates coming in after the timeout won't be exchanged with the remote peer. But I guess if you want to exchange candidates after an offer, you should use trickle.

LGTM. Great analysis too.

[t-mullen]

We don't usually commit the build until a release.

[chr15m]

@t-mullen thanks for your review! I've update the PR to remove the build artifact.

[DiegoRBaquero]

@feross needed.