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

Convert many requests from Node/Rust to worker into notification #1121

Open
ibc opened this issue Jul 17, 2023 · 2 comments
Open

Convert many requests from Node/Rust to worker into notification #1121

ibc opened this issue Jul 17, 2023 · 2 comments
Labels
Milestone

Comments

@ibc
Copy link
Member

ibc commented Jul 17, 2023

It doesn't make sense to send requests in some cases where a response is not expected. For example:

  • pause/resume() methods in Producer, Consumer, DataProducer and DataConsumer.

Imagine that the app calls consumer.pause(). Imagine that just before the "pause" request reaches the worker, the producer was closed and hence the consumer is already closed/freed in the worker, although the "producerclose" notification didn't reach yet Node land.

  • In v3 this means that consumer.pause() will throw (because the consumer is closed), and this will force the user to write . catch()` etc.
  • In v4, consumer.pause() should send a notification to the worker and immediately set the paused flag to true in Node/Rust layers.
  • Same for other classes and methods.
  • A good example of why this is needed is to avoid these issues: Fix issue when pause/resume events are not emitted #1231
@ibc ibc added the feature label Jul 17, 2023
@ibc ibc added this to the v4 milestone Jul 17, 2023
@ibc ibc mentioned this issue Jul 17, 2023
7 tasks
@nazar-pc
Copy link
Collaborator

I think the benefit of request/response is that we get an acknowledgement back. If we are fine to not wait for acknowledgement then we can switch to notifications indeed.

@ibc
Copy link
Member Author

ibc commented Jul 18, 2023

Specially in flatbuffers branch (so I assume in v4 too) we use a single channel to communicate Node and worker, and it's a UnixSocket in stream mode, meaning that message order and delivery is guaranteed. So once you send a "data producer pause" notification you know that the next time you call dataProducer.send() the C++ DataProducer will be pause when that message reaches the worker. So no need for worker acknowledge here. Note: this is not true for RTP or DataChannel packets received from the network but it doesn't matter IMHO.

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

No branches or pull requests

2 participants