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

Support for getting peers and closing transports #12

Merged
merged 1 commit into from
Mar 10, 2021

Conversation

keianhzo
Copy link
Contributor

@keianhzo keianhzo commented Feb 18, 2021

This PR adds support for closing transports from a client and getting the other peers consumers. This is used int he hubs client to recreate transports and consumers/producers when we need to recover from a ICE failure.

Related Hubs-Foundation/hubs#3930

Copy link

@netpro2k netpro2k left a comment

Choose a reason for hiding this comment

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

Still reviewing the client side PR but this mostly looks good. I do think we should move the MediaSoup update to its own PR, and made some suggestions on some possible cleanup.

lib/Room.js Outdated
@@ -816,6 +816,74 @@ class Room extends EventEmitter
break;
}

case 'getPeers':

Choose a reason for hiding this comment

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

When reading the client code I was pretty confused that the result of this wasnt being used. I did not realize it actually ended up creating consumers. Maybe we should name it something like consumeExistingProducers? We might also want oto use this on joining rather than doing that automatically.

Alternatively, it might be nice to actually just return the list of existing producing peers and have the client request to create producers. (both in this case, and the join case)... Though that might be better for another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to refactor existing producers common code but I think it's ok leaving the flow as it is where consumers for existing producers are returned inline, otherwise it's another message that we have to exchange and traverse again the whole peers map for something that we always are going to need to to in both cases.

Maybe I up change the method name to refreshConsumers. The returned peers are used in the join to hold the connection flow until all consumers are received but in this case it doesn't really makes much sense.

lib/Room.js Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@keianhzo keianhzo merged commit eaee7b8 into Hubs-Foundation:master Mar 10, 2021
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.

2 participants