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

Fix deadlocked engine in RPC #532

Merged
merged 8 commits into from
Jan 3, 2025
Merged

Fix deadlocked engine in RPC #532

merged 8 commits into from
Jan 3, 2025

Conversation

bcherry
Copy link
Contributor

@bcherry bcherry commented Dec 19, 2024

There's an issue where if you're busy handling an incoming rpc request, additional engine events will not be processed. this causes a deadlock essentially if you have nested RPC, since the incoming acks won't be processed, but also probably gums up plenty of other features of livekit...

this solution is probably not correct, but it works and i included an example. need help @typester !

Copy link
Contributor

ilo-nanpa bot commented Dec 19, 2024

it seems like you haven't added any nanpa changeset files to this PR.

if this pull request includes changes to code, make sure to add a changeset, by writing a file to .nanpa/<unique-name>.kdl:

minor type="added" "Introduce frobnication algorithm"

refer to the manpage for more information.

@typester
Copy link
Member

Interesting. Will take a look!

@typester
Copy link
Member

The change you made (d7c8cda) seems like a good fix to me 👍

@typester
Copy link
Member

What caught my attention in a different context is the await here:

task.await;

If we await at this point, engine_events will be processed one by one. To me, it seems more natural not to await here, but what was the intention behind this design?

@bcherry bcherry requested a review from theomonnom December 20, 2024 18:08
@bcherry
Copy link
Contributor Author

bcherry commented Dec 20, 2024

@typester I'm not sure about that, maybe @theomonnom knows

@theomonnom
Copy link
Member

yeah, it is expected for engine events to be processed one by one (keeping the order)

@bcherry bcherry merged commit 8e2d312 into main Jan 3, 2025
11 of 16 checks passed
@bcherry bcherry deleted the bcherry/nested-rpc branch January 3, 2025 20:04
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.

3 participants