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

Only switch to failed after all candidates are exhausted? #271

Open
jech opened this issue Sep 5, 2020 · 7 comments
Open

Only switch to failed after all candidates are exhausted? #271

jech opened this issue Sep 5, 2020 · 7 comments

Comments

@jech
Copy link
Member

jech commented Sep 5, 2020

As pointed out in pion/webrtc#1212 (comment) , Pion's handling of the failed state is somewhat naive: Pion will switch to failed if connectivity cannot be established even if it hasn't received an end-of-candidates indication.

There's a tradeoff here: the current behaviour works well enough in practice, and it gracefully handles the case when the peer never sends an end-of-candidates indication. On the other hand, it means that if the peer is performing expensive gathering (e.g. because a TURN server is overloaded), we might spuriously switch into failed.

@scorpionknifes
Copy link
Member

@Sean-Der just clarifying, it should only fail if end-of-candidate is detected? if so how do we handle it gracefully if the peer never sends an end-of-candidate indication?

Cause it would break this test (potentially implementation).

ice/agent_test.go

Line 1307 in 62d1c40

func TestConnectionStateConnectingToFailed(t *testing.T) {
.

@Sean-Der
Copy link
Member

Hey @scorpionknifes

Sorry for the confusion, but I don't think we can move forward on this. Behavior between browsers is inconsistent. I am afraid to implement something and then have browsers diverge. I am sorry I didn't do more research before recommending this issue :(

@Sean-Der
Copy link
Member

We should love your PR though, and maybe engage with webrtc-pc on what is the proper behavior?

@jech
Copy link
Member Author

jech commented Dec 6, 2020

Small timeout when eoc is received, larger timeout if it isn't?

@scorpionknifes
Copy link
Member

@jech cool idea. idk if this is a feature people want?

@jech
Copy link
Member Author

jech commented Dec 7, 2020

There's a tradeoff here: on the one hand, we don't want to give up too early, on the other hand, we want to switch to failed as early as possible so as to give the user timely feedback and possibly trigger an ICE restart. Question: would using the end-of-candidates indication allow us to fail faster without breaking connectivity in cases where it currently works?
I'm hoping somebody will do the necessary experiments, and I sure hope that won't be me ;-)

Sean-Der pushed a commit that referenced this issue Dec 9, 2020
Allow a user to pass a nil Candidate. We perform no actions off of this
currently. Until browsers implement end-of-candidates consistently it
isn't something we can do.

Relates to pion/webrtc#1212 and #271
@jech
Copy link
Member Author

jech commented Dec 10, 2020

There is some related discussion at https://tools.ietf.org/html/draft-ietf-ice-trickle-21#section-14.

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