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

Refactor internal #889

Merged
merged 5 commits into from
Aug 22, 2022
Merged

Refactor internal #889

merged 5 commits into from
Aug 22, 2022

Conversation

nazar-pc
Copy link
Collaborator

The goal of this PR is to simplify things by replacing internal data structure that has several IDs, unique for different use cases, with one handlerId that can be looked up in a global map instead.

In order to do that, every entity in the worker now implements Channel::ChannelSocket::RequestHandler if it handles Channel requests, PayloadChannel::PayloadChannelSocket::RequestHandler if it handles PayloadChannel requests and PayloadChannel::PayloadChannelSocket::NotificationHandler if it supports notifications. With that we can store them in 3 global maps and look up by handleId in one step instead of traversing down into nested structure.

While this simplifies handling somewhat, it is even more beneficial for re-implementing #870 on top since we will no longer need to worry about a list of IDs that can belong to anything, we'll always have exactly one ID to deal with at a time.

Reviewing commits separately is recommended.

@nazar-pc nazar-pc requested review from ibc and jmillan August 18, 2022 09:45
Copy link
Member

@ibc ibc left a comment

Choose a reason for hiding this comment

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

Still not completely reviewed, just some comments added. Another one here: shouldn't we add proper TS type to internal object received by JS instances such as Consumer?

node/src/PipeTransport.ts Show resolved Hide resolved
node/src/PipeTransport.ts Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

Another one here: shouldn't we add proper TS type to internal object received by JS instances such as Consumer?

Potentially, but nothing has changed in that regard, internal field in classes is the same as before, it just isn't sent in full to worker anymore. I think we can do that separately if you'd like.

node/src/PipeTransport.ts Show resolved Hide resolved
node/src/PipeTransport.ts Outdated Show resolved Hide resolved
node/src/Transport.ts Outdated Show resolved Hide resolved
node/src/Router.ts Outdated Show resolved Hide resolved
node/src/Router.ts Outdated Show resolved Hide resolved
node/src/Router.ts Outdated Show resolved Hide resolved
node/src/Router.ts Outdated Show resolved Hide resolved
node/src/Transport.ts Outdated Show resolved Hide resolved
node/src/Transport.ts Outdated Show resolved Hide resolved
node/src/Transport.ts Outdated Show resolved Hide resolved
node/src/Worker.ts Outdated Show resolved Hide resolved
node/src/Worker.ts Outdated Show resolved Hide resolved
Co-authored-by: Iñaki Baz Castillo <ibc@aliax.net>
@nazar-pc nazar-pc requested a review from ibc August 21, 2022 21:07
@ibc
Copy link
Member

ibc commented Aug 22, 2022

Can this be merged so I'll adapt pending work of Jose in a new PR?

@nazar-pc nazar-pc merged commit 11f45ff into versatica:v3 Aug 22, 2022
@nazar-pc nazar-pc deleted the refactor-internal branch August 22, 2022 08:57
@MatanYemini
Copy link

prod.id 66d90c9f-3792-412c-b566-c861c7d0d1f7 error Error: Channel request handler with ID 66d90c9f-3792-412c-b566-c861c7d0d1f7 already exists [method:transport.produce]","time":"2022-10-15T13:51:02.831Z","v":0}

I am getting this error constantly after these changes.

Seems to happen once I subscribe "too fast", if I wait 5-10s, seems to work. (after I was told that it is available)

Will provide more details..

@nazar-pc
Copy link
Collaborator Author

nazar-pc commented Oct 16, 2022

@MatanYemini make sure to not pipe producers within the same worker to avoid that error. If you have further comments please create a thread on the forum: https://mediasoup.discourse.group/

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

Successfully merging this pull request may close these issues.

3 participants