Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detecting ICE connection that will never connect #253

Closed
ArmorDarks opened this issue Mar 24, 2023 · 3 comments · Fixed by #276
Closed

Detecting ICE connection that will never connect #253

ArmorDarks opened this issue Mar 24, 2023 · 3 comments · Fixed by #276

Comments

@ArmorDarks
Copy link

Feature Request

Problem

Under normal circumstances, in case of connection issues, the ICE connection should eventually fail. connectionstatechange event will fire, and the application will have the opportunity to do something with it.

However, if all ICE candidates error during the gathering, the connection will forever stay in a new state and connectionstatechange event will never fire.

Example:

image

It will be possible to produce and consume, but actually, no data will be transmitted since there's no live candidate pair.

The client can check periodically for candidate pairs in stats and timeout, but there is a better way by using RTCPeerConnection.onicegatheringstatechange event.

If ICE iceGatheringState changes to complete and connectionState is still new - that means it's a dead end unless restartIce() will be invoked. It's a good opportunity for the application to warn the user that something went wrong.

However, right now, mediasoup-client doesn't expose the iceGatheringState, nor onicegatheringstatechange handler.

Proposal

Option 1: Expose the onicegatheringstatechange event and iceGatheringState on the Transport so that application can check for it.

Option 2: Add a new Transport event which will fire on pc.iceGatheringState === 'complete' && pc.connectionState === 'new' that will indicate a connection that can't be established

Caveats

onicegatheringstatechange has good browser support. If we want to cover even more browsers, we can rely on icecandidate event and event.candidate === null, which also indicates that the gathering is finished.

@ibc
Copy link
Member

ibc commented Apr 16, 2023

Will address in next weeks. Too busy with many other things. Thanks.

@thebongy
Copy link
Contributor

Hi @ibc , can I take this change up in a PR?

About the gatheringState, we can have an API exactly similar to the connectionState, this should be straightfoward. (Along with the backward comatibility logic using the iceCandidate event as @ArmorDarks pointed out)

As for option 2, we can discuss this further, but this can be a seperate change, as doing option 1 itself would let users implement such checks in their code.

ibc added a commit that referenced this issue Jul 17, 2023
Fixes #253

- Add `transport.iceGatheringState` getter.
- Add `transport.on('iceGatheringStateChangeEventNumTimesCalled', (iceGatheringState) => { })` event,
- Bonus track: Reset `transport.connectionState` to "closed" within `transport.close()` method.

*NOTE:* This is done in all handlers. It may happen that super old ones (`Chrome55`) don't support `icegatheringstatechange` event. I won't do magic. We'll remove those super old handlers soon anyway.
@ibc
Copy link
Member

ibc commented Jul 17, 2023

Sorry @thebongy, I just got 15 minutes and did it. #276

@ibc ibc closed this as completed in #276 Jul 17, 2023
ibc added a commit that referenced this issue Jul 17, 2023
Fixes #253

- Add `transport.iceGatheringState` getter.
- Add `transport.on('iceGatheringStateChangeEventNumTimesCalled', (iceGatheringState) => { })` event,
- Bonus track: Reset `transport.connectionState` to "closed" within `transport.close()` method.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants