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

Implement restartIce functionality? #190

Closed
jjmaldonis opened this issue Sep 27, 2019 · 3 comments
Closed

Implement restartIce functionality? #190

jjmaldonis opened this issue Sep 27, 2019 · 3 comments

Comments

@jjmaldonis
Copy link

Summary

Are you planning to support the ICERestart feature? It's available as an option in the OfferOptions struct, but options are not allowed as a parameter to PeerConnection.CreateOffer.

Motivation

Currently the only way to reconnect to a WebRTC session is to tear down the peer connection and recreate it on both sides. There's quite a bit of communication overhead required to do that, which is normally taken care of by the ICERestart flag.

Google's WebRTC implementation implements the ICE restart functionality.

I don't know if this is on your radar right now or not.

Describe alternatives you've considered

Repeated from above: "Currently the only way to reconnect to a WebRTC session is to tear down the peer connection and recreate it on both sides. There's quite a bit of communication overhead required to do that, which is normally taken care of by the ICERestart flag."

Additional Context

N/A

@Sean-Der
Copy link
Member

Hey @jjmaldonis thanks for checking out Pion!

Yes I would like to support it. I also don't believe it would be that much development work either.

We will need to evaluate if it is easier to restart an ICEAgent (or create a whole new one). I am leaning toward restarting it, so others can benefit from the change.

My goal is have 100% coverage of the webrtc-pc RFC! If you are interested/have time I would love to support you getting ICERestarts in. If not I think this can probably happen before the end of the year. I have already obligated myself to a few other big features :)

@jjmaldonis
Copy link
Author

Thanks that's great. I'm not sure if I can commit to implementing the iceRestart flag right now. If that changes, I'll let you know. I'm really looking forward to it though, and thanks for the awesome library and all your hard work!

@normanr
Copy link

normanr commented Apr 9, 2020

What's the expect action to take if a DataChannel.Send() fails due to connection failure? (sending payload data in non-established state: state=Closed). Should that DataChannel 'heal' once the connection state goes back to connected? or should the calling code Close() the channel, and reopen a new one?

@Sean-Der Sean-Der transferred this issue from pion/webrtc May 23, 2020
Sean-Der added a commit that referenced this issue Jun 20, 2020
Implementing failed state and ICE Restarts requires breaking changes

Relates to #190
Sean-Der added a commit that referenced this issue Jun 20, 2020
ICE has two distinct states `Disconnected` and `Failed`. We need to
split out these arguments so that we can control (and test) both.

Relates to #190
Sean-Der added a commit that referenced this issue Jun 21, 2020
Implementing failed state and ICE Restarts requires breaking changes

Relates to #190
Sean-Der added a commit that referenced this issue Jun 21, 2020
ICE has two distinct states `Disconnected` and `Failed`. We need to
split out these arguments so that we can control (and test) both.

Relates to #190
Sean-Der added a commit that referenced this issue Jun 22, 2020
Selection before was written in a way that wouldn't work for restarts.
This moves the code out and makes it stateless. In the future we will be
able to clear the candidates and update the ConnectionState and
everything will restart properly.

Relates to #190
Sean-Der added a commit that referenced this issue Jun 22, 2020
Selection before was written in a way that wouldn't work for restarts.
This moves the code out and makes it stateless. In the future we will be
able to clear the candidates and update the ConnectionState and
everything will restart properly.

Relates to #190
Sean-Der added a commit that referenced this issue Jun 22, 2020
Selection before was written in a way that wouldn't work for restarts.
This moves the code out and makes it stateless. In the future we will be
able to clear the candidates and update the ConnectionState and
everything will restart properly.

Relates to #190
Sean-Der added a commit that referenced this issue Jun 24, 2020
Add simple tests that check it works E2E

Resolves #190
Sean-Der added a commit that referenced this issue Jun 24, 2020
Callers of run need to be able to cancel their waiting individually.
This would cause hung threads during restart if we attempted to close
a candidate while processing a packet for it.

Before the run would be canceled by the global close, but we can't
depend on that anymore.

Resolves #190
Sean-Der added a commit that referenced this issue Jun 25, 2020
This should be lower then defaultDisconnectTimeout otherwise
we are going to enter disconnected.

Resolves #190
Sean-Der added a commit that referenced this issue Jun 25, 2020
This should be lower then defaultDisconnectTimeout otherwise
we are going to enter disconnected.

Also rename defaultDisconnectTimeout -> defaultDisconnectedTimeout
to make the tense consistent with other options

Resolves #190
Sean-Der added a commit that referenced this issue Jun 25, 2020
Selection before was written in a way that wouldn't work for restarts.
This moves the code out and makes it stateless. In the future we will be
able to clear the candidates and update the ConnectionState and
everything will restart properly.

Relates to #190
Sean-Der added a commit that referenced this issue Jun 25, 2020
Callers of run need to be able to cancel their waiting individually.
This would cause hung threads during restart if we attempted to close
a candidate while processing a packet for it.

Before the run would be canceled by the global close, but we can't
depend on that anymore.

Resolves #190
Sean-Der added a commit that referenced this issue Jun 25, 2020
This should be lower then defaultDisconnectTimeout otherwise
we are going to enter disconnected.

Also rename defaultDisconnectTimeout -> defaultDisconnectedTimeout
to make the tense consistent with other options

Resolves #190
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants