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

webrtc(private-to-private): leave the signalling stream open on successful connection #580

Closed
wants to merge 3 commits into from

Conversation

achingbrain
Copy link
Member

After a successful connection we should leave the signalling stream open - this is to renegotiate ICE candidates if required.

Refs:

@achingbrain achingbrain changed the title webrtc: leave the signalling stream open on successful connection webrtc(private-to-private): leave the signalling stream open on successful connection Sep 26, 2023
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me. Thank you @achingbrain.

@sukunrt given that you are working on libp2p/go-libp2p#2576, looks good to you?

webrtc/webrtc.md Outdated Show resolved Hide resolved
Co-authored-by: Marten Seemann <martenseemann@gmail.com>
@thomaseizinger
Copy link
Contributor

This means the underlying relayed connection also can't close, correct? Do we expect nodes to reestablish the signalling stream in that case?

Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced anymore that this is the right way to go. In fact, what you're trying to support is connection migration, but this only works if both connections to the relay also survive the migration event. Looking at the Web transports:

  • This definitely won't work with WebSocket.
  • It could work with the WebTransport transport, but it requires the QUIC stacks to support the connection migration.
  • It could work with WebRTC.

Keeping a connection, especially a relayed one, seems like a pretty high price to pay for a somewhat obscure scenario. There's a lot of value in freeing up the relay to help other nodes on the network connect instead.

@achingbrain
Copy link
Member Author

This means the underlying relayed connection also can't close, correct?

Correct, yes.

Looking at the Web transports

Yes, I think this would only apply to WebRTC, that is, the negotiationneeded event doesn't seem to have an equivalent in the WebSockets or WebTransport APIs AFAICT.

Keeping a connection, especially a relayed one, seems like a pretty high price to pay for a somewhat obscure scenario. There's a lot of value in freeing up the relay to help other nodes on the network connect instead.

An alternative way to handle it would be to just open a new relayed connection if renegotiation is required? When the new incoming /webrtc-signaling stream is opened I guess you'd just check if you already have a RTCPeerConnection for that peer and use the incoming ICE candiates to renegotiate it rather than create a new one from scratch.

Or just do nothing and throw?

@thomaseizinger
Copy link
Contributor

Keeping a connection, especially a relayed one, seems like a pretty high price to pay for a somewhat obscure scenario. There's a lot of value in freeing up the relay to help other nodes on the network connect instead.

An alternative way to handle it would be to just open a new relayed connection if renegotiation is required? When the new incoming /webrtc-signaling stream is opened I guess you'd just check if you already have a RTCPeerConnection for that peer and use the incoming ICE candiates to renegotiate it rather than create a new one from scratch.

Or just do nothing and throw?

I assume today's behaviour is that all streams on the current connection fail and the entire connection is marked as failed. That could happen for any reason so the application has to have some kind of re-connection scheme anyway, right?

Instead of leaving the stream open, it could be useful to specify that open a new signaling stream (which will likely imply making a new relayed connection).

I'd assume that browser nodes have an open reservation with at least one relay most of the time in order to allow new connections. Thus, establishing a new stream should be pretty quick.

@achingbrain
Copy link
Member Author

I assume today's behaviour is that all streams on the current connection fail and the entire connection is marked as failed.

Right now, yes - if the connectionState changes to anything that isn't connecting or connected we treat it as failure and destroy the libp2p connection/any open streams on that connection.

so the application has to have some kind of re-connection scheme anyway, right?

In general yes, but WebRTC gives us the tools to attempt to handle this transparently at the transport layer.

I'd assume that browser nodes have an open reservation with at least one relay most of the time in order to allow new connections. Thus, establishing a new stream should be pretty quick.

Maybe, whatever disrupted the WebRTC session may have disrupted the relay connection too, plus if you were the dialler you may have closed the relay connection already.

Another use case here is mobile. If you're walking around running js-libp2p on react-native or whatever it would be pretty common to associate with a new cell tower which would likely involve a network change without (I think) much of a period of inaccessibility.

Again though, that may also disrupt your connection to the relay.

I think in the short term maybe we should just close the signalling stream and have the connection subject to the regular rules on connection culling, we can always revisit when we have more data.

@achingbrain
Copy link
Member Author

I'm going to close this, we can revisit later if necessary.

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

Successfully merging this pull request may close these issues.

None yet

5 participants